Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ADD][16.0][MIG] account_move_substate #1568

Merged
merged 8 commits into from
Mar 13, 2024
Merged

[ADD][16.0][MIG] account_move_substate #1568

merged 8 commits into from
Mar 13, 2024

Conversation

bosd
Copy link
Contributor

@bosd bosd commented Oct 4, 2023

standard addition / migration 😃

@bosd bosd force-pushed the 16.0-mig-account_move_substate branch from 0255c54 to 8b1ebdc Compare October 4, 2023 21:26
@bosd bosd force-pushed the 16.0-mig-account_move_substate branch from 8b1ebdc to 3e41e62 Compare October 4, 2023 21:28
@bosd bosd marked this pull request as ready for review October 4, 2023 21:39
@bosd
Copy link
Contributor Author

bosd commented Oct 5, 2023

@CasVissers-360ERP Can you please review?

CasVissers-360ERP
CasVissers-360ERP approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review.

Dranyel-Bosd

This comment was marked as duplicate.

Sign in to view

Saran440
Saran440 reviewed Oct 24, 2023
View reviewed changes
account_move_substate/models/account_move.py
_name = "account.move"
_state_field = "state"

def _track_template(self, changes):
Copy link
Member

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

Copy link
Contributor

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?

Copy link
Contributor Author

@bosd bosd Nov 2, 2023 •
edited

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@bosd bosd Nov 2, 2023 •
edited

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

@bosd
Copy link
Contributor Author

bosd commented Nov 2, 2023

@etobella Can you please merge this one? 🙏

@etobella
Copy link
Member

etobella commented Nov 2, 2023

We shall wait for the merge on server-ux, isn't it?

@bosd
Copy link
Contributor Author

bosd commented Nov 2, 2023 •
edited

We shall wait for the merge on server-ux, isn't it?

@etobella We could wait, but IMO better to merge it as is.
This PR follows the same structure as all other _substate modules.
Example: sale_substate for V16 which has been merged.

If/when the refactor of the base_substate module happens,
We should refactor all the _substate modules. (This one, sale_substate, purchase_substate and so on)

Waiting..... is also blocking backports from this pr. 😉

@bosd
Copy link
Contributor Author

bosd commented Nov 6, 2023

@Saran440 Do you concur to merge this one as is. And deal with refactoring later when/if it happens?

@Saran440
Copy link
Member

Saran440 commented Nov 7, 2023

@bosd I see your commit from v14, edit template and migrate to v15. but v15 is don't have PR?

Should we waiting v14 merge first? #1001
next, we will migrate from v14 to v16 (if v15 no one used)

@bosd
Copy link
Contributor Author

bosd commented Nov 7, 2023

but v15 is don't have PR?

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)

@bosd
Copy link
Contributor Author

bosd commented Nov 7, 2023

Oops, had a long day.
Forgot I made the pr yesterday see #1595 😀

Dranyel-Bosd
Dranyel-Bosd approved these changes Nov 7, 2023
View reviewed changes
Copy link

@Dranyel-Bosd Dranyel-Bosd left a 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 👍

@Saran440 Saran440 mentioned this pull request Jan 26, 2024
Open
@bosd
Copy link
Contributor Author

bosd commented Mar 13, 2024

We shall wait for the merge on server-ux, isn't it?

@etobella I think we've waited long enough 😄
Can you 🙏 merge? We can always deal with any refactor later.

etobella
etobella approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@etobella etobella left a comment •
edited

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

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Mar 13, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Mar 13, 2024
Open
60 tasks
@etobella
Copy link
Member

/ocabot migration account_move_substate

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-1568-by-etobella-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 13, 2024
@OCA-git-bot
Merge PR #1568 into 16.0
80c21b8 
Signed-off-by etobella
@etobella
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1568-by-etobella-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b324da7 into OCA:16.0 Mar 13, 2024
4 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7165d18. Thanks a lot for contributing to OCA. ❤️

@bosd bosd deleted the 16.0-mig-account_move_substate branch April 27, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CasVissers-360ERP CasVissers-360ERP CasVissers-360ERP approved these changes

@Saran440 Saran440 Saran440 left review comments

@etobella etobella etobella approved these changes

@Dranyel-Bosd Dranyel-Bosd Dranyel-Bosd approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@bosd @etobella @Saran440 @OCA-git-bot @CasVissers-360ERP @Dranyel-Bosd @ps-tubtim