-
-
Notifications
You must be signed in to change notification settings - Fork 22
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][ADD] cooperator_website_recaptcha #44
base: 14.0
Are you sure you want to change the base?
[14.0][ADD] cooperator_website_recaptcha #44
Conversation
Signed-off-by: Carmen Bianca Bakker <[email protected]>
Signed-off-by: Carmen Bianca Bakker <[email protected]>
Signed-off-by: Carmen Bianca Bakker <[email protected]>
Signed-off-by: Carmen Bianca Bakker <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
@carmenbianca or @huguesdk can you have a look at the tests ? |
and in #43 (comment) as well |
@robinkeunen these tests fail because they depend on OCA/website#928 which is not merged (but ci passes). i was about to ping a maintainer to merge it, but i saw that the ci on the 12.0 version of that branch (OCA/website#928) fails because of OCA/maintainer-quality-tools#712. i’ve just created OCA/maintainer-quality-tools#715 to fix it. let’s see if this will make things move forward. |
8cc7e82
to
c9ee8bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (functional and code)
I see the refacto will prevent from being able to configure the recaptcha by company. Is it an issue with the new multi-company functionalities ?
Todo :
|
6d0b07d
to
977bf4e
Compare
the only thing that was moved out of the company was a boolean controlling whether recaptcha is enabled or not. however, previously, the recaptcha keys were stored on the website, as we can see here. this was changed to remove the dependency on i think you’re right regarding multi-company. the 3 config settings should probably be stored on the company (or on the website). recaptcha keys work per domain. i don’t know yet how websites work in multi-company mode, but if different companies use different domains, they will need different recaptcha keys. maybe introduce a new module depending on this can be handled separately though, as it is an improvement to the current state. |
@huguesdk OK for handling separately |
Turns out there is a standard Odoo recaptcha module in 14, should we use it rather than portal_recaptcha ? |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
* move the recaptcha widget view to portal_recaptcha. * remove res.company.captcha_type (portal_recaptcha can now be enabled and disabled in the settings). * override WebsiteSubscription._additional_validate() instead of .validation().
fix author and website manifest properties to follow oca's guidelines.
use website_recaptcha_v2 instead of portal_recaptcha.
977bf4e
to
928548c
Compare
i’ve refactored the module to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can commits be squashed please.
@dreispt since this is a move from our non-oca repository to here, i think keeping the history is important (same as for a migration pr). could you please help with getting OCA/website#1030 merged (on which this pr depends)? |
move
cooperator_website_recaptcha
module from coopiteasy/vertical-cooperative to the oca.this branch contains an extra refactoring commit (from the 14.0-ref-cooperator_website_recaptcha_to_portal_recaptcha branch) that should be reviewed. this refactoring depends on #42 and OCA/website#928.