-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[T-6536][15.0][ADD]transfer_client_portfolio, transfer_client_portfolio_commissions: Add new modules #14
Conversation
dd68313
to
e71aa3c
Compare
e71aa3c
to
3a91f38
Compare
18b9177
to
355ca75
Compare
4ec405e
to
c02b33b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional Review: LGTM 👍🏻
@manuelregidor Can you review? |
@Tisho99 Pre-commit is failing. Please, let me know when it's fixed and I'll do the technical review. And do you think it is necessary to have three commits? |
@manuelregidor None of the pre-commit errors are due to my module, in that case the PR can be approbed, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small comments. No need to make changes if you don't think you need to.
c02b33b
to
fe912f1
Compare
transfer_client_portfolio_commissions/wizard/trasnfer_portfolio_wizard.py
Outdated
Show resolved
Hide resolved
fe912f1
to
da8db9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Tisho99 Would it take you a long time to fix the bugs indicated in the pre-commit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix pre-commit
1 hour more or less, there are a lot of errors |
db8ab3d
to
67bb993
Compare
Hi @ValentinVinagre @manuelregidor I have fixed pre-commit. However, i am not sure if the following fix is correct: Pre-commit told me i have to delete the cr.commit() and refactor the code if it was necessary to do the deletion. I have only deleted the cr.commit() because the write above should update the database. What do you think? |
@Tisho99 I don't think the write method writes on the database straightaway, but only after the transactions has finished. If there was an error, all the write operations performed before would be discarded, as the update query would not be triggered. |
cr.commit() was performed because:
|
67bb993
to
4858ab3
Compare
I have put again the cr.commit and skipped the pre-commit error. This can be merged. You have the activity in T-6536 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
This module allows to transfer contacts, activities and opportunities from one user to another.
The pre-commit error are caused by other modules
T-6536
@HaraldPanten
@ValentinVinagre
@luis-ron