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.

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

[15.0][MIG] account_menu: migrate module from 14.0 to 15.0 #1275

Merged
merged 32 commits into from
Apr 4, 2022

Conversation

AlvaroTForgeFlow
Copy link
Contributor

No description provided.

@AlvaroTForgeFlow
Copy link
Contributor Author

@ForgeFlow

Copy link
Contributor

@ChrisOForgeFlow ChrisOForgeFlow left a comment

Choose a reason for hiding this comment

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

Please let us evaluate the feasibility of renaming, since the module has several additional features to only the menus.

@@ -0,0 +1,2 @@
* Suggest to rename to something like `account_usability` in 15.0, given that
Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree with this recommendation

Copy link
Contributor Author

@AlvaroTForgeFlow AlvaroTForgeFlow Dec 3, 2021

Choose a reason for hiding this comment

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

Understood. I don't think a renaming is needed since menus are still the main functionality, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, they are the menu feature, however the module has more features than the menu, so the name change could give more sense to the rest of the added features, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. I'll make sure to make those changes asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChrisOForgeFlow It should be good now.

Copy link
Contributor

@ChrisOForgeFlow ChrisOForgeFlow left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@HaraldPanten HaraldPanten 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

Copy link
Contributor

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Please, check pre-commit

@AlvaroTForgeFlow
Copy link
Contributor Author

Oddly enough, I tried to re-run it in local and the pre-commit passed succesfully.

@GuillemCForgeFlow GuillemCForgeFlow force-pushed the 15.0-mig-account_menu branch 2 times, most recently from 11c714e to 98759a0 Compare March 14, 2022 08:50
@GuillemCForgeFlow
Copy link

@HaraldPanten It seems like the runboat is failing because of this error when building:
× python setup.py egg_info did not run successfully. │ exit code: 1 ╰─> [1 lines of output] error in vatnumber setup command: use_2to3 is invalid. [end of output]
I've seen other PR's of this repo which seem to have the same error. Any idea on how should we fix this?

@HaraldPanten
Copy link
Contributor

Hhhhmmm... maybe @sbidoul or @pedrobaeza could shed some light here.

@sbidoul
Copy link
Member

sbidoul commented Mar 14, 2022

@GuillemCForgeFlow @HaraldPanten the problem here is that base_vat_optional_vies depends on vatnumber, which is unmaintained and incompatible with the latest setuptools.

image

Odoo itself has removed the dependency on vatnumber. I think we should do the same, as we don't want to pin an old setuptools version in the oca/oca-ci image just for this.

@rafaelbn
Copy link
Member

/ocabot migration account_menu

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone Mar 23, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Mar 23, 2022
31 tasks
@rafaelbn
Copy link
Member

We have the same problem about vatnumber in #1360

@rafaelbn
Copy link
Member

rafaelbn commented Apr 4, 2022

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

@rafaelbn The rebase process failed, because command git push --force ForgeFlow tmp-pr-1275:15.0-mig-account_menu failed with output:

remote: Permission to ForgeFlow/account-financial-tools.git denied to OCA-git-bot.
fatal: unable to access 'https:/ForgeFlow/account-financial-tools/': The requested URL returned error: 403

@rafaelbn
Copy link
Member

rafaelbn commented Apr 4, 2022

Hello @HaraldPanten , please could you re-review? 😄 this is ready!

@rafaelbn
Copy link
Member

rafaelbn commented Apr 4, 2022

@AaronHForgeFlow please could you review this PR from @ForgeFlow ? thanks!

@rafaelbn
Copy link
Member

rafaelbn commented Apr 4, 2022

Solved

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@rafaelbn
Copy link
Member

rafaelbn commented Apr 4, 2022

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-1275-by-rafaelbn-bump-nobump, awaiting test results.

Copy link
Contributor

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

LGTM

@OCA-git-bot OCA-git-bot merged commit cdf14f4 into OCA:15.0 Apr 4, 2022
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.