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

[14.0][MIG] website_sale_require_login: Migration to 14.0 #866

Merged
merged 24 commits into from
Dec 31, 2023

Conversation

njeudy
Copy link

@njeudy njeudy commented Nov 1, 2023

#464 seems to be closed, but need this module.

  • I start from initOS branch
  • rebase it to last 14.0 from oca
  • add address controler in blocked addresses.

Don't now if it's a good way to propose again this PR ..

yajo and others added 23 commits November 1, 2023 09:18
Example of module which requires such refactoring:

https://github.com/it-projects-llc/website-addons/tree/10.0/website_sale_checkout_store

[FIX] condition to show normal checkout button was wrong in website_sale_suggest_create_account
I was equal to

(user_authenticated or not signup_allowed and can_checkout)

while it has to be

(user_authenticated or not signup_allowed) and can_checkout
This test was expecting sign up to be disabled because `auth_signup` is not in its module graph, but since that addon is autoinstallable, it could be installed without our knowledge.

Just make sure signup is disabled before running the public user test, to make it unitary.

Also, moved to at-install mode, since the post-install one adds no value.
@njeudy
Copy link
Author

njeudy commented Nov 1, 2023

Think pre-commit error is not related to me ? can anyone explain what can I do ?

@njeudy
Copy link
Author

njeudy commented Nov 1, 2023

@Tardo @pedrobaeza can you help me to review this ?

@pedrobaeza
Copy link
Member

For pre-commit, #864 should be finished. For the tests of this PR, check what is the problem.

@njeudy
Copy link
Author

njeudy commented Nov 1, 2023

For pre-commit, #864 should be finished. For the tests of this PR, check what is the problem.

Don't undestand ui js test .. when I try manually, it work, think I should find if the selector is correct ..

@pedrobaeza
Copy link
Member

Look at 15.0 to see the changes done there if they are applicable here.

@njeudy njeudy force-pushed the 14.0-mig-website_sale_require_login branch from aff7e3a to b0935d5 Compare November 1, 2023 19:25
@Tisho99
Copy link

Tisho99 commented Dec 29, 2023

Hello @njeudy , are you willing to continue this PR?

I also have interest on migrating this module

Copy link

@Tisho99 Tisho99 left a comment

Choose a reason for hiding this comment

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

LGTM Functional and technical review

@njeudy njeudy force-pushed the 14.0-mig-website_sale_require_login branch from b0935d5 to 117ec8c Compare December 31, 2023 05:28
@njeudy
Copy link
Author

njeudy commented Dec 31, 2023

I think this PR is ready now, just force push to restart all action. @pedrobaeza is it correct for you ?

@pedrobaeza pedrobaeza added this to the 14.0 milestone Dec 31, 2023
@pedrobaeza
Copy link
Member

Although your commit messages are not correct (and they should be both in one), let's merge it being something relatively old:

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-866-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 58fba80 into OCA:14.0 Dec 31, 2023
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at dd45473. 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.