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

[16.0][MIG] website_sale_suggest_create_account: Migration to version 16.0 #836

Merged

Conversation

pilarvargas-tecnativa
Copy link
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa commented Aug 30, 2023

This PR supersedes #705

cc @Tecnativa TT44387

@chienandalu @CarlosRoca13 please review :)

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-mig-website_sale_suggest_create_account branch 5 times, most recently from e0bedb6 to de55c3a Compare August 31, 2023 06:43
Copy link
Contributor

@CarlosRoca13 CarlosRoca13 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pedrobaeza
Copy link
Member

/ocabot migration website_sale_suggest_create_account

@pedrobaeza pedrobaeza requested a review from chienandalu August 31, 2023 06:59
@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Aug 31, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Aug 31, 2023
33 tasks
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Can you push again to get a runbot?

@pedrobaeza
Copy link
Member

Runboat is not working from yesterday. GH is returning 422 error to the runboat service, as @sbidoul commented yesterday, but now there's no bad status on hooks according status page.

@chienandalu
Copy link
Member

Oh, thanks. I'll test locally then

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

I'd simplify it. To me, it makes more sense this behavior:

Sign in/up checkout:

  • optional -> Sign in/up and checkout
  • disabled -> Sign in and checkout button
  • mandatory -> Sign in/up and checkout (the same one that in the first place)

Indeed you just need to change the button text

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-mig-website_sale_suggest_create_account branch from de55c3a to 6d49b50 Compare August 31, 2023 15:01
@pilarvargas-tecnativa
Copy link
Contributor Author

I'd simplify it. To me, it makes more sense this behavior:

Sign in/up checkout:

  • optional -> Sign in/up and checkout
  • disabled -> Sign in and checkout button
  • mandatory -> Sign in/up and checkout (the same one that in the first place)

Indeed you just need to change the button text

I have made the change in the button names but in the "mandatory" option it is not allowed to checkout without being registered/logged in, by default the login button appears and I have also added the registration button.

In short, the three options are:

  • Optional: allows checkout as guest/register/log in (v15 free sign up). Button: sign in/up and checkout
  • Disable: allow checkout as guest/log in (v15 on invitation). Button: sign in and checkout
  • Mandatory: do not allow guest checkout/allow sign in and login (the functionality of website_sale_require_login). Button: sign up (sign in is already by default)

@chienandalu
Copy link
Member

  • Mandatory: do not allow guest checkout/allow sign in and login (the functionality of website_sale_require_login). Button: sign up (sign in is already by default)

Then just change the original text to Sign in/up. The place for the user to decide whether to create a new account or to log in with an existing one is /web/login

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-mig-website_sale_suggest_create_account branch from 6d49b50 to f70de7d Compare September 1, 2023 06:49
@pilarvargas-tecnativa
Copy link
Contributor Author

/web/login

The changes are already made to both the button name/link and the template, so it would appear in the "Mandatory" option:
image

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thanks! :)

Copy link
Contributor

@CarlosRoca13 CarlosRoca13 left a comment

Choose a reason for hiding this comment

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

By making these changes we are adding the functionality of the https://github.com/OCA/e-commerce/tree/15.0/website_sale_require_login module to this one, right?

If so, please add the necessary migration scripts.

@pilarvargas-tecnativa
Copy link
Contributor Author

By making these changes we are adding the functionality of the https://github.com/OCA/e-commerce/tree/15.0/website_sale_require_login module to this one, right?

If so, please add the necessary migration scripts.

website_sale_require_login has been merged into website_sale, here is the migration script:
OCA/OpenUpgrade#4113

@CarlosRoca13
Copy link
Contributor

Thx 😄

@pilarvargas-tecnativa
Copy link
Contributor Author

@pedrobaeza Can this be merged?

@pedrobaeza
Copy link
Member

pedrobaeza commented Oct 18, 2023

You should squash administrative commits and the translations commits from the same author (not mixing languages).

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-mig-website_sale_suggest_create_account branch from f70de7d to 7f360cf Compare October 18, 2023 15:28
@pilarvargas-tecnativa
Copy link
Contributor Author

You should squash administrative commits and the translations commits from the same author (not mixing languages).

done, thanks :)

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@pilarvargas-tecnativa
Copy link
Contributor Author

So the error persists... It seems Odoo has changed recently something, and it's not still propagated to OCB. Can you please check deeper?

But now the error is in the tests, the same one that comes and goes as it pleases. I investigate and replicate locally to see what I can find out.

@pedrobaeza
Copy link
Member

It only fails persistently in Odoo one (not OCB). You locally have OCB. Please check with Odoo latest source code to see.

ovnicraft and others added 19 commits October 19, 2023 20:02
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
Always show 'Process Checkout', not highlighted if user not logged in
Suggest to login or signup, according to 'invitation_scope', avoiding too many buttons in cart page
Currently translated at 100.0% (4 of 4 strings)

Translation: e-commerce-12.0/e-commerce-12.0-website_sale_suggest_create_account
Translate-URL: https://translation.odoo-community.org/projects/e-commerce-12-0/e-commerce-12-0-website_sale_suggest_create_account/es/
Ensure that 'vat' is not empty for compatibility with website_sale_vat_required module
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 16.0-mig-website_sale_suggest_create_account branch from 02393c2 to e6a75bc Compare October 19, 2023 18:02
@pedrobaeza
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-836-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3bb06ec into OCA:16.0 Oct 19, 2023
4 of 6 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 16.0-mig-website_sale_suggest_create_account branch October 19, 2023 18:25
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.