Skip to content

Conversation

@trisdoan
Copy link
Contributor

@trisdoan trisdoan commented Mar 6, 2024

No description provided.

Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

LGTM. Functional review.

("DEBT", "Borne by Debtor"),
],
default="SLEV",
readonly=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you think it would be more convenient to leave this line? It should always be readonly except when the state is draft or open, which is defined in the view. In my opinion, it would be better to keep the field as readonly in the logic layer, as editting the view manually can be easily done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @manuelregidor, thank you for your in-depth review.

readonly=True prevents users from importing. That's why I removed it

Copy link
Member

Choose a reason for hiding this comment

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

Then put the readonly in view

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, it's the first time I hear about this limitation. Is it something new in 17.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

@trisdoan @pedrobaeza yes, it's new in v17

Copy link
Member

Choose a reason for hiding this comment

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

Then as said, you should limit it in the view.

"debtor.",
)
batch_booking = fields.Boolean(
readonly=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as charge_bearer field.

@pedrobaeza
Copy link
Member

/ocabot migration account_banking_pain_base

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Mar 11, 2024
@HaraldPanten
Copy link

Hi @trisdoan will you continue with the suggested changes?

@trisdoan
Copy link
Contributor Author

Hi @HaraldPanten and @pedrobaeza , please correct me if I understood you wrong

I think I applied the read-only for batch_booking and charge_bearer in the view

  <field name="batch_booking" readonly="state not in ['draft','open']" />
  <field
      name="charge_bearer"
      invisible="sepa"
      readonly="state not in ['draft','open']"
  />

@pedrobaeza
Copy link
Member

Yes, that's it.

Copy link
Member

@angelmoya angelmoya left a comment

Choose a reason for hiding this comment

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

👍

@pedrobaeza
Copy link
Member

@trisdoan are you going to perform the latest change we commented?

@trisdoan
Copy link
Contributor Author

Hi @pedrobaeza, sorry I am a bit confused now 😕 I might miss something.

are you going to perform the latest change we commented?

I assume you mean: read-only for batch_booking and charge_bearer in the view ? If yes, I already did that

Or not, can you help to show me what I miss, I am willing to adjust it

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Sorry, that's it!

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1231-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 22, 2024
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-1231-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@pedrobaeza
Copy link
Member

@trisdoan please rebase and check pre-commit errors:

account_banking_pain_base/models/account_payment_order.py:63:89: E501 Line too long (175 > 88)
account_banking_pain_base/models/account_payment_order.py:229:89: E501 Line too long (92 > 88)

@OCA-git-bot
Copy link
Contributor

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-1231-by-pedrobaeza-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

Alexis de Lattre and others added 7 commits March 22, 2024 22:35
…13-11-21:

- add module account_banking_pain_base
- add support for initiating party identification, priority and structured remittance info in XML file
- the requested execution date now uses the fields date_prefered and date_scheduled (the field of the wizard has been removed)
- the 'convert to ascii' feature is now an option of the payment mode (enabled by default)
- set 'communication' field of payment.line to 140 chars and hide field 'communication2'
- Code factoring for SCT and SDD
- Add support for direct debit migration from national format to SEPA
- Source : Standard-XML-SDD-Initiation-v3-EN by Febelfin
- Add group in order to mask the fields for "Original Mandate Indentification" for users in countries that don't required it.
- Add tracking on important fields of the mandate.
- Better form view on company.
- Update constraint on mandate following my devs on "Original Mandate Identification".
- Add support for Party Identifier for Belgium. Can be easily extended for other countries.
- Mutualize more code between SCT and SDD.
* Make it easier to extend structured communication types.
* Use new style to make selection extendable.
* Help text for bank and BIC fields. Courtesy of Alexis de Lattre.
* Do not use InstrPrty for Direct Debit.
pedrobaeza and others added 21 commits March 22, 2024 22:35
Currently translated at 64.7% (110 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/es/
Currently translated at 98.8% (168 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/es/
Currently translated at 60.0% (102 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/hr/
Currently translated at 100.0% (170 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/fr/
Currently translated at 31.1% (53 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Currently translated at 100.0% (170 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/es/
Currently translated at 31.1% (53 of 170 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/
The field 'sepa' on account.payment.order is only display for SEPA
payment methods.
If the option "show warning if not SEPA" is enabled on the payment
method, a warning banner is now displayed on payment orders with a SEPA
payment method which are not SEPA.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/
Currently translated at 100.0% (175 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/es/
Currently translated at 30.2% (53 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Currently translated at 30.8% (54 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Currently translated at 30.8% (54 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Currently translated at 30.8% (54 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Currently translated at 30.8% (54 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
Currently translated at 30.8% (54 of 175 strings)

Translation: bank-payment-16.0/bank-payment-16.0-account_banking_pain_base
Translate-URL: https://translation.odoo-community.org/projects/bank-payment-16-0/bank-payment-16-0-account_banking_pain_base/it/
@trisdoan trisdoan force-pushed the 17.0-mig-account_banking_pain_base branch from e5267b7 to 6294587 Compare March 22, 2024 15:48
@trisdoan
Copy link
Contributor Author

ping @pedrobaeza, it's done

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-1231-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 7948ccb into OCA:17.0 Mar 22, 2024
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 09650c3. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.