-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
[15.0][IMP] payment_redsys: remove website_sale dependency #3804
[15.0][IMP] payment_redsys: remove website_sale dependency #3804
Conversation
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 👍🏻
@@ -105,7 +105,7 @@ def _get_website_url(self): | |||
or self.env.context.get("website_id") | |||
and self.env["website"].browse(self.env.context["website_id"]) | |||
) | |||
domain = website and website.domain or "" | |||
domain = website and website.domain |
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.
?Por qué quitar el or ""
? Así, puedes tener un False
pululando por ahí que dé error.
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.
Eso diría que se tendría que haber preguntado en el PR original de https://github.com/OCA/l10n-spain/pull/3777/files 😝 ya que realizaron ellos el cambio. Podemos volverlo a añadir en esta versión si es necesario.
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.
Ya, ahí no revisé yo como vi 3 aprobados y lo único que hice fue fusionar, pero no debería quitarse. Si no, no tiene sentido tampoco el and
y bastaría con website.domain
. Restituidlo por favor.
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.
te parece que se haga en un segundo commit por si se quiere pasar a las superiores?
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.
No merece la pena un commit aparte.
…eration to make web.base.url the default if website is not installed
12b5729
to
3ee60ac
Compare
@Tisho99 LGTM! Probado en entorno local. |
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.
/ocabot merge patch
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 0fb9b09. Thanks a lot for contributing to OCA. ❤️ |
Backward Port of #3777
T-6685