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

[13.0][MIG] website_sale_require_legal: Migration to 13.0 #443

Merged
merged 19 commits into from
Jul 15, 2021

Conversation

ernestotejeda
Copy link
Member

cc @Tecnativa TT25963

@pedrobaeza pedrobaeza requested review from Tardo and yajo October 7, 2020 17:44
@ernestotejeda ernestotejeda force-pushed the 13.0-mig-website_sale_require_legal branch from e0ef4d7 to 8cb6f9a Compare October 7, 2020 22:00
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.

Tours are failing

<xpath expr=".">
<script
type="text/javascript"
src="/website_sale_require_legal/static/src/js/tour.js"
Copy link
Member

Choose a reason for hiding this comment

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

Move tour.js to a new location: /website_sale_require_legal/static/tests/tours/tour.js

@ernestotejeda ernestotejeda force-pushed the 13.0-mig-website_sale_require_legal branch from 8cb6f9a to 9f2d564 Compare October 13, 2020 16:33
@ernestotejeda
Copy link
Member Author

@pedrobaeza , I have applied CSS to the label of the checkbox to be in red if it is not checked. I think that with that, we don't have to modify website_legal_page (OCA/website#782) so the translations of this module will not be affected.

@pedrobaeza
Copy link
Member

Shouldn't the CSS be applied on parent module?

@ernestotejeda
Copy link
Member Author

I don't think so, In this case the selector of the css rule is based on the input that is created in this module

@ernestotejeda ernestotejeda force-pushed the 13.0-mig-website_sale_require_legal branch 2 times, most recently from cc2f8a9 to fdec6c1 Compare October 14, 2020 03:30
@sergio-teruel
Copy link
Contributor

Is needed this module??
You can customize this option in odoo core checkout step
https://github.com/odoo/odoo/blob/905fe5af69094051b9f7bf433a9c83cc0953fde1/addons/website_sale/views/templates.xml#L1489

@ernestotejeda
Copy link
Member Author

Yes, maybe this module is not necessary. Ping @pedrobaeza . Confirm this to me to do the corresponding changes in OpenUpgrade
Peek 2020-10-15 10-54

@pedrobaeza
Copy link
Member

Please show me screenshots of the existing feature and the module to compare

@ernestotejeda
Copy link
Member Author

This is how this module works:
Peek 2020-10-15 11-04

@sergio-teruel
Copy link
Contributor

I think that with the core function it's ok

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.

The core function looks good, but we should check if metadata is still being registered (

def _checkout_form_save(self, mode, checkout, all_values):
res = super()._checkout_form_save(mode, checkout, all_values)
if all_values.get("submitted") and all_values.get("accepted_legal_terms"):
environ = request.httprequest.headers.environ
metadata = "Website legal terms acceptance metadata: "
metadata += "<br/>".join(
"{}: {}".format(val, environ.get(val))
for val in ("REMOTE_ADDR", "HTTP_USER_AGENT", "HTTP_ACCEPT_LANGUAGE",)
)
partner_id = request.env["res.partner"].browse(res)
website_user = request.website.salesperson_id.id or SUPERUSER_ID
partner_id.with_user(website_user).message_post(
body=metadata, message_type="notification"
)
return res
) as it's a legal requirement in some countries.

@sergio-teruel
Copy link
Contributor

@chienandalu What do you propose?

@chienandalu
Copy link
Member

To check if it's covered. Otherwise, we should do it (maybe in other module)

@ernestotejeda
Copy link
Member Author

@chienandalu , I have reviewed and as I have seen, with the Odoo core functionality, this metadata is not sent either when the address user form is modified or when the payment is done, no matter if the checkbox of the terms and terms is checked.

@pedrobaeza
Copy link
Member

I think you haven't showed how standard works (or the video was for the standard?). And if so, how the other works? I want to compare? Anyway, that metadata should be saved, so this module can remain for at least that.

@ernestotejeda
Copy link
Member Author

@pedrobaeza
Copy link
Member

Ok, thanks. Then let's keep this module for recording that metadata.

@ernestotejeda ernestotejeda force-pushed the 13.0-mig-website_sale_require_legal branch from fdec6c1 to af10f0d Compare October 19, 2020 17:11
@ernestotejeda
Copy link
Member Author

@sergio-teruel @chienandalu @pedrobaeza Ready to review.

Copy link
Contributor

@sergio-teruel sergio-teruel left a comment

Choose a reason for hiding this comment

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

I have a doubt...
We are going to use the core check but recording the metadata... Is that so?

======================

* Shopping terms and conditions are accepted only on user registration or
address edition. So if those terms change after the user signed up, a
Copy link
Contributor

Choose a reason for hiding this comment

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

With the new behavior, the checkbox is displayed in checkout step.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sergio-teruel , the Odoo (core) option shows the check box always just before paying, but what this point of the readme refers to is to show that box only if the terms have changed (perhaps to be shown before paying as well). I think they are two different things. And it seems to me that to cover that point (if we needed), we must add the functionality in this module, because the odoo option is not very configurable since it is a template designed to be activated and deactivated by the UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think?

@pedrobaeza
Copy link
Member

@ernestotejeda please check this

@ernestotejeda
Copy link
Member Author

@pedrobaeza @sergio-teruel @chienandalu
What are we going to do with this PR?

@HaraldPanten
Copy link

Hi,

Any news about this module? Finally we're going to keep it in OCA, right? @pedrobaeza

@yajo yajo force-pushed the 13.0-mig-website_sale_require_legal branch from 857e039 to 8437ce8 Compare July 12, 2021 13:35
@yajo
Copy link
Member

yajo commented Jul 12, 2021

Last commit applies all that has been discussed and tests it. Also, rebased. Please review again. Thanks!

@pedrobaeza
Copy link
Member

Travis is red

@yajo yajo force-pushed the 13.0-mig-website_sale_require_legal branch from 8437ce8 to 18f9a6e Compare July 13, 2021 07:33
…cope

This is what the module does now:

- Add the posibility to configure legal terms acceptance requirement before saving a new partner.
- Log that acceptance as a note in the partner.
- Extend upstream's legal requirement before payment and log that acceptance as a note in the sale order.

@Tecnativa TT25963
@yajo yajo force-pushed the 13.0-mig-website_sale_require_legal branch from 18f9a6e to 6b9c2ba Compare July 13, 2021 07:33
@yajo
Copy link
Member

yajo commented Jul 13, 2021

Thanks, it should be fixed now.

@yajo yajo requested review from chienandalu, sergio-teruel, yajo and pedrobaeza and removed request for yajo July 13, 2021 09:12
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.

👍 Tested

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-443-by-pedrobaeza-bump-nobump, awaiting test results.

@pedrobaeza pedrobaeza mentioned this pull request Jul 15, 2021
22 tasks
@OCA-git-bot OCA-git-bot merged commit 21bf8d4 into OCA:13.0 Jul 15, 2021
@OCA-git-bot
Copy link
Contributor

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

@yajo yajo deleted the 13.0-mig-website_sale_require_legal branch July 16, 2021 06:42
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.

None yet