Skip to content

[17.0][IMP] account_payment_mode: change type of payment_mode.note field from Text to Html#1445

Merged
OCA-git-bot merged 1 commit intoOCA:17.0from
Tecnativa:17.0-fix-account_payment_mode-note
Apr 14, 2025
Merged

[17.0][IMP] account_payment_mode: change type of payment_mode.note field from Text to Html#1445
OCA-git-bot merged 1 commit intoOCA:17.0from
Tecnativa:17.0-fix-account_payment_mode-note

Conversation

@pilarvargas-tecnativa
Copy link
Copy Markdown
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa commented Apr 14, 2025

fw-port of #1360

@Tecnativa TT56010

@pedrobaeza @sergio-teruel please review

Copy link
Copy Markdown
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.

/ocabot merge minor

@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

@OCA-git-bot OCA-git-bot merged commit d425a4a into OCA:17.0 Apr 14, 2025
7 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

@pedrobaeza pedrobaeza deleted the 17.0-fix-account_payment_mode-note branch April 14, 2025 21:34
@etobella
Copy link
Copy Markdown
Member

@pedrobaeza @pilarvargas-tecnativa Shouldn't this need a migration script?

@pilarvargas-tecnativa
Copy link
Copy Markdown
Contributor Author

pilarvargas-tecnativa commented Apr 15, 2025

@pedrobaeza @pilarvargas-tecnativa Shouldn't this need a migration script?

Hi @etobella, this change was made in v16 with his script, when migrating the module to v17 this commit had not been included so the bd's that have already been migrated to v17 have the field as text and contain something like <p> Text </p> and it is displayed like this literally in the field and in the reports. When testing by running the script on the migrated field in v17 with html tags in the text, it is not interpreted and the text is stored and displayed like this: <p>&lt;p&gt;Text&lt;/p&gt;</p>
For that reason the script already running in v16 has been removed.

@etobella
Copy link
Copy Markdown
Member

👍

I didn't notice that it comes from a Forward Port. Thanks for the clarification

@etobella
Copy link
Copy Markdown
Member

Actually, After giving 5 seconds of thought the Scripts are necessary. Imagine that you started on 17. Then the field was text, and after this change, it might be broken.

@pilarvargas-tecnativa
Copy link
Copy Markdown
Contributor Author

Actually, After giving 5 seconds of thought the Scripts are necessary. Imagine that you started on 17. Then the field was text, and after this change, it might be broken.

Right, different scenarios:

  • Start in v17 with the text field (in this case the script would work).
  • Coming from v16 and having in v17 the html field as text with the tags (the script would break the data).
  • Be still in v16 and move to v17 (in this case it would not be necessary).

Within these scenarios, if you add the script otherwise, you have to redo by hand in one of the first two. How do you decide which is the priority?

@etobella
Copy link
Copy Markdown
Member

Well, there are options for this. For example:

1- Check the migration version (not the best option, but it might work in 90% of the cases)
2- Check if the field has html tags

@pedrobaeza
Copy link
Copy Markdown
Member

The second is not very feasible, and the first one may have several variables. Please go ahead with the script if you have it clear.

@etobella
Copy link
Copy Markdown
Member

Using the following query might work (tested with other fields and tables)

SELECT id from account_payment_mode where note::text ~* '<[^>]+>' LIMIT 1

If it is OK for you, I can do the migration script.

@pedrobaeza
Copy link
Copy Markdown
Member

OK, go ahead with it, which seems good enough

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.

6 participants