-
-
Notifications
You must be signed in to change notification settings - Fork 664
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
[ADD][16.0][MIG] account_move_substate #1568
Conversation
add substate to tree view, group buy and search and fix form view
[IMP] account_move_substate: add tests
0255c54
to
8b1ebdc
Compare
8b1ebdc
to
3e41e62
Compare
@CasVissers-360ERP Can you please review? |
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.
_name = "account.move" | ||
_state_field = "state" | ||
|
||
def _track_template(self, changes): |
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.
This function will add on base_substate. Can you delete this please
OCA/server-ux#736
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.
@bosd can you take a look here?
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.
Will do, Let's first get OCA/server-ux#736 merged. As this one depends on it.
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.
@Saran440 Can you re-review? 🙏
Please make it non blocking. As your proposed refactor is not included yet for V16.
There is still ongoing discussion on the refactor. It should not block this PR.
We can always refactor this module, once the proposed changes are in.
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 noticed this is non blocking
@etobella Can you please merge this one? 🙏 |
We shall wait for the merge on server-ux, isn't it? |
@etobella We could wait, but IMO better to merge it as is. If/when the refactor of the Waiting..... is also blocking backports from this pr. 😉 |
@Saran440 Do you concur to merge this one as is. And deal with refactoring later when/if it happens? |
I am at Odoo XP. Can open the pr for v15 when I have a bit of free time. 😉 Because it can take some time to get a merge. I opened the migration commits as well. If no one uses or reviews v15, we just let it go stale and closed. (As sadly happens with more pr's) |
Oops, had a long day. |
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.
Looks so good to me 👍
@etobella I think we've waited long enough 😄 |
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.
I do agree with you @bosd
We can proceed with merge, because the other module is just blocking it but seems that there will not be any news soon....
/ocabot merge nobump
/ocabot migration account_move_substate |
This PR looks fantastic, let's merge it! |
/ocabot merge nobump |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 7165d18. Thanks a lot for contributing to OCA. ❤️ |
standard addition / migration 😃