-
-
Notifications
You must be signed in to change notification settings - Fork 414
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
[IMP] SUITE account payment return: Improvements to suite #107
[IMP] SUITE account payment return: Improvements to suite #107
Conversation
def unlink(self): | ||
if self.filtered(lambda x: x.state == 'done'): | ||
raise UserError(_( | ||
"You can not be removed a payment return if state is 'Done'")) |
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.
Remove be
#, python-format | ||
msgid "" | ||
"Could not make sense of the given file.\n" | ||
"Did you install the module to support this type of file?" | ||
msgstr "No se pudo hacer con sentido de el archivo dado.\n¿Instalaste el modulo para soportar este tipo de archivo?" | ||
msgstr "" | ||
"No se pudo hacer con sentido de el archivo dado.\n" |
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.
Trato de usted, no de tú
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.
Leo cosas contradictorias al respecto, por eso nunca lo tengo claro...
@@ -42,6 +42,10 @@ | |||
<field name="code">MD01</field> | |||
<field name="name">No valid Mandate</field> | |||
</record> | |||
<record id="unpaid_reason_MD06" model="payment.return.reason"> | |||
<field name="code">MD06</field> | |||
<field name="name">Disputed authorised transaction</field> |
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.
This isn't translated?
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.
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.
OK
f3739fc
to
da46560
Compare
Include a test for unlinking returns for not dropping coverage |
da46560
to
5232871
Compare
@pedrobaeza changes done |
Please don't squash while the review process. You only need to squash before merging (when the PR is approved), or it's more difficult to track changes from one review to the next. Reviewer can indicate you if you need to squash or not. |
You have to test also a case where there's no error and the payment can be removed. |
008d6da
to
80eab8f
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.
Thanks for the changes.
To the merger: squash on merging
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.
OK! Thanks
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.
Ok..
if self.filtered(lambda x: x.state == 'done'): | ||
raise UserError(_( | ||
"You can not remove a payment return if state is 'Done'")) | ||
return super(PaymentReturn, self).unlink() |
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.
I'd implement this as a global ir.rule instead.
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.
It is an option, but usually this is the way used and possibly better performance.
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.
The problem is the overlapping of access rules that grants you at the end the unlink. This has to be the way.
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.
Well, I know it's late and after all it works fine this way, but just to clarify, I meant a global rule. Quoting from the above link:
Global rules are subtractive, they must all be matched for a record to be accessible
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.
OK, thanks for clarifying. Odoo doesn't do this kind of things by security rules, so there should be a reason for that. Maybe performance as pointed by Carlos, or maybe to not bypass this check removing access rule.
Merging |
@Tecnativa