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

11.0 mig website_sale_require_legal #237

Merged
merged 7 commits into from
Jun 18, 2018
Merged

11.0 mig website_sale_require_legal #237

merged 7 commits into from
Jun 18, 2018

Conversation

ernestotejeda
Copy link
Member

@ernestotejeda ernestotejeda commented Jun 4, 2018 •
edited
Loading

Normal migration

  • Update README

Cc @Tecnativa

chienandalu
chienandalu suggested changes Jun 4, 2018
View reviewed changes
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 for your work @ernestotejeda You should migrate from 10.0 though (https://github.com/OCA/e-commerce/tree/10.0/website_sale_require_legal) wich has several improvements like gdpr compliance, etc.

@rafaelbn rafaelbn added this to the 11.0 milestone Jun 5, 2018
@rafaelbn
Copy link
Member

rafaelbn commented Jun 5, 2018 •
edited
Loading

Thanks @ernestotejeda !

@Tecnativa previous PR:

This is a set of @gdpr improvements

@ernestotejeda ernestotejeda force-pushed the 11.0-mig-website_sale_require_legal branch from 05d64c1 to de2310b Compare June 12, 2018 14:15
@rafaelbn
Copy link
Member

Please @chienandalu review again, thanks!

yajo
yajo approved these changes Jun 12, 2018
View reviewed changes
website_sale_require_legal/readme/CONTRIBUTORS.rst Outdated
* David Vidal <david.vidal@tecnativa.com>
* `Tecnativa <https://www.tecnativa.com>`_:

* Ernesto Tejeda <ernesto.tejeda87@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

All the above are from tecnativa too, put them here.

website_sale_require_legal/readme/INSTALL.rst
@@ -0,0 +1,3 @@
To install this module, you need to:

* Install repository `OCA/website <https://github.com/OCA/website>`_.
Copy link
Member

Choose a reason for hiding this comment

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

This is implicit, remove it please. (People get dependencies when downloading from apps.odoo.com)

Copy link
Member Author

Choose a reason for hiding this comment

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

In PR #238 @chienandalu comment something like this, and @pedrobaeza answered:
That's not true. You have to indicate dependencies outside of the current repo, which is the case.

Please, check that and confirm me what i should do, thanks.

@rafaelbn
Copy link
Member

@ernestotejeda please review runbot warning:

2018-06-12 15:12:26,448 169 WARNING openerp_test odoo.addons.base.ir.ir_ui_view: Error-prone use of @class in view address (): use the hasclass(*classes) function to filter elements by their classes
2018-06-12 15:12:26,460 169 INFO openerp_test odoo.modules.loading: loading website_sale_require_legal/demo/assets.xml
2018-06-12 15:12:26,537 169 WARNING openerp_test odoo.addons.base.ir.ir_ui_view: Error-prone use of @class in view address (website_sale_require_legal.address): use the hasclass(*classes) function to filter elements by their classes

yajo
yajo requested changes Jun 13, 2018
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Almost perfect, just one last detail.

website_sale_require_legal/controllers/main.py Outdated
all_values.get('accepted_legal_terms')):
environ = request.httprequest.headers.environ
metadata = "Website legal terms acceptance metadata: "
metadata += "\n".join(
Copy link
Member

Choose a reason for hiding this comment

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

Use <br/> instead. This is what you get now:

captura de pantalla de 2018-06-13 11-21-49

@rafaelbn
Copy link
Member

Hi @ernestotejeda tested functionally it doesn't work. There in not legal advice and checkbox:

2018-06-13_15-21-55

rafaelbn
rafaelbn requested changes Jun 13, 2018
View reviewed changes
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Need fixing

rafaelbn
rafaelbn approved these changes Jun 13, 2018
View reviewed changes
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Sorry, someone uninstall de module....
Tested functionality 👍

@rafaelbn
Copy link
Member

Please @ernestotejeda add test

codecov/patch — 65.62% of diff hit (target 100%)
codecov/project — 75% (-25%) compared to 14117f2

Thank you

@rafaelbn rafaelbn removed the approved label Jun 13, 2018
@yajo
Copy link
Member

yajo commented Jun 14, 2018 •
edited
Loading

Awesome. Please squash migration commits, to merge. Thanks! 😉

@ernestotejeda ernestotejeda force-pushed the 11.0-mig-website_sale_require_legal branch from dbc5c73 to 42c3305 Compare June 14, 2018 23:14
@pedrobaeza pedrobaeza merged commit 490bc3d into OCA:11.0 Jun 18, 2018
@pedrobaeza pedrobaeza mentioned this pull request Jun 18, 2018
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yajo yajo yajo approved these changes

@chienandalu chienandalu chienandalu approved these changes

@rafaelbn rafaelbn rafaelbn approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
11.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ernestotejeda @rafaelbn @yajo @chienandalu @pedrobaeza @cubells @oca-transbot @grindtildeath