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

[12.0][MIG] website_sale_require_legal: Migration to v12 #267

Merged
merged 11 commits into from
Apr 22, 2019

Conversation

cristinamartinrod
Copy link
Member

@cristinamartinrod cristinamartinrod mentioned this pull request Feb 26, 2019
19 tasks
@pedrobaeza pedrobaeza added this to the 12.0 milestone Feb 26, 2019
@cristinamartinrod cristinamartinrod marked this pull request as ready for review March 1, 2019 08:58
@cristinamartinrod cristinamartinrod force-pushed the 12.0-mig-website_sale_require_legal branch from c8032e5 to 49ee3f8 Compare March 1, 2019 10:41
@cristinamartinrod cristinamartinrod force-pushed the 12.0-mig-website_sale_require_legal branch from 49ee3f8 to 0c28f68 Compare March 1, 2019 11:21
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.

Code looks OK, but just a couple of details:

captura de pantalla de 2019-03-01 11-56-58

Apart from that, clicking any of them while unauthenticated directs me to a 404 error:

captura de pantalla de 2019-03-01 11-57-11

@cristinamartinrod
Copy link
Member Author

cristinamartinrod commented Mar 1, 2019

@yajo This is something from v12. It's because there are times that the cart can be very long and this a more visible option in the part on the right. And I suppose that the use of different colors is to only focus attention in one of them, that is usually something normal.

The 404 error seems to be something of runbot because if you uninstall the module it continues to happen and it doesn't seem to have any additional associated permission in the module. On local I didn't detected that error. Could it be that? Is it crazy?

@yajo
Copy link
Member

yajo commented Mar 1, 2019

This is something from v12. It's because there are times that the cart can be very long and this a more visible option in the part on the right. And I suppose that the use of different colors is to only focus attention in one of them, that is usually something normal.

True, I didn't see it before!

OK then.

The 404 error seems to be something of runbot because

Possibly you need to enable signup in the website. I think it's done from its settings.

I don't know if it's done now via ir.config_parameter for all websites, or is it done per website. Check it out and:

  • If it's global, enable it automatically when installing the addon.
  • If it's per website, add the instructions to the configuration section of the README, and do not display the button in websites that have it disabled.

@pedrobaeza
Copy link
Member

Shouldn't we activate it when installing this module?

@yajo
Copy link
Member

yajo commented Mar 1, 2019

Yes, if it's global, but if it's per website, then IMHO we should leave that option untouched.

@pedrobaeza
Copy link
Member

If not, it should be properly documented

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.

The tour is failing. You'll have to update some step probably.

@cristinamartinrod cristinamartinrod force-pushed the 12.0-mig-website_sale_require_legal branch 3 times, most recently from 5194f6f to c292c82 Compare March 8, 2019 08:18
website_sale_require_legal/static/src/js/tour.js Outdated Show resolved Hide resolved
website_sale_require_legal/static/src/js/tour.js Outdated Show resolved Hide resolved
website_sale_require_legal/static/src/js/tour.js Outdated Show resolved Hide resolved
website_sale_require_legal/static/src/js/tour.js Outdated Show resolved Hide resolved
website_sale_require_legal/views/website_sale.xml Outdated Show resolved Hide resolved
@cristinamartinrod cristinamartinrod force-pushed the 12.0-mig-website_sale_require_legal branch from c292c82 to 5ef716e Compare March 13, 2019 08:34
@cristinamartinrod cristinamartinrod force-pushed the 12.0-mig-website_sale_require_legal branch 4 times, most recently from 187b16e to b1d88da Compare April 11, 2019 07:28
@yajo yajo force-pushed the 12.0-mig-website_sale_require_legal branch from b1d88da to 5117267 Compare April 16, 2019 12:23
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.

Tests fixed

@pedrobaeza
Copy link
Member

I'm afraid not

@yajo
Copy link
Member

yajo commented Apr 17, 2019

I restarted https://travis-ci.org/OCA/e-commerce/builds/437949375 because I suspect it's the main branch that's broken.

@pedrobaeza
Copy link
Member

Yes, main branch is also red. Please check and fix it.

@yajo
Copy link
Member

yajo commented Apr 18, 2019

Fix in #281.

@pedrobaeza
Copy link
Member

Please rebase

@yajo yajo force-pushed the 12.0-mig-website_sale_require_legal branch from 5117267 to 049a787 Compare April 22, 2019 07:18
@yajo
Copy link
Member

yajo commented Apr 22, 2019

Done, let's see travis.

@pedrobaeza pedrobaeza merged commit 83a7413 into OCA:12.0 Apr 22, 2019
@pedrobaeza pedrobaeza deleted the 12.0-mig-website_sale_require_legal branch April 22, 2019 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants