Skip to content
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] edi: refactor model rules w/ specific model #797

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Jul 5, 2023

Long awaited improvement.
Allows to decouple rules by model giving much more flexibility.
Migration steps included.
Backward compat kept.

@OCA-git-bot
Copy link
Contributor

Hi @etobella,
some modules you are maintaining are being modified, check this out!

@simahawk
Copy link
Contributor Author

simahawk commented Jul 5, 2023

@etobella can you please have a look? I know you might have existing implementations for Spanish localization that rely on form btns and I'd like not to break them if possible :)

@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from 0b586d2 to 941e046 Compare July 5, 2023 11:46
@etobella
Copy link
Member

etobella commented Jul 5, 2023

I did not override that function, but the change might break installation, as the fields might be used on data 🤔
I know you added a migration script, but might be a problem...

https://github.com/OCA/l10n-spain/blob/14.0/l10n_es_facturae_face/data/edi.xml#L16-L21

@simahawk
Copy link
Contributor Author

simahawk commented Jul 5, 2023

I did not override that function, but the change might break installation, as the fields might be used on data thinking I know you added a migration script, but might be a problem...

https://github.com/OCA/l10n-spain/blob/14.0/l10n_es_facturae_face/data/edi.xml#L16-L21

I see. What would you suggest? Can't we update those files? Shouldn't be that hard.

Other options:

  1. full backward compat (eg: keep the fields and rely on the rule to lookup for conf on the type + hide fields on type form via specific group)
  2. leave fields, hide and ignore them (they will get migrated anyway) + log deprecation

I would like to avoid both of them honestly 😜

@etobella
Copy link
Member

etobella commented Jul 6, 2023

My suggestion would be to leave them for 14, 15 and 16, but make them non storable and store the data properly on the new table. We need to make an inverse and compute in order to fill the data and fields properly 🤔

I know it is much harder, but it is probably cleaner, WDYT?

@simahawk
Copy link
Contributor Author

simahawk commented Jul 6, 2023

My suggestion would be to leave them for 14, 15 and 16, but make them non storable and store the data properly on the new table. We need to make an inverse and compute in order to fill the data and fields properly thinking

I know it is much harder, but it is probably cleaner, WDYT?

Ok for 14 and 15 but definitely not for 16 (which is not even ready yet).

I'm not in favor of keeping full support for the old style tho:

  1. hide fields, to not break those files
  2. deprecate it, to force users to switch (maybe even w/ a nice warning on the form)
  3. add an inverse field only to reflect updates if necessary if you keep using the outdated version in such files

Sounds good?
Otherwise let's make a call (like in the old days) 😉

@simahawk
Copy link
Contributor Author

@etobella ping :)

@etobella
Copy link
Member

Good for me @simahawk If it is not merged on 16, we can remove the old way there 😉

@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from de25b3b to 310b935 Compare August 10, 2023 13:23
@simahawk
Copy link
Contributor Author

simahawk commented Aug 10, 2023

@etobella backward compat layer added. Tests included. Could you please test on your side when you have time?

image

@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from 310b935 to a4814fd Compare August 10, 2023 13:41
@simahawk
Copy link
Contributor Author

Small improvement: a btn to wipe fields carefully

image

@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from a4814fd to 4e3bfc7 Compare August 10, 2023 13:59
@simahawk simahawk mentioned this pull request Aug 12, 2023
2 tasks
@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from 4e3bfc7 to a60dca5 Compare August 12, 2023 10:28
Copy link
Contributor

@OriolMForgeFlow OriolMForgeFlow left a comment

Choose a reason for hiding this comment

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

Hi @simahawk ,
I am encountering an error during testing in runboat, as I am unable to create Exchange Type Rules, since the field type_id is required but is not displayed within the form view.

@simahawk
Copy link
Contributor Author

Hi @simahawk , I am encountering an error during testing in runboat, as I am unable to create Exchange Type Rules, since the field type_id is required but is not displayed within the form view.

Sorry, I missed a commit. Pushed now

@simahawk
Copy link
Contributor Author

@etobella could you drop a review here pls?

@etobella
Copy link
Member

I will test it this weekend 😁

@johnny-longneck
Copy link

Hey Simone, I said I will also look at it this weekend but at the moment I will leave it to Etobella,

I was checking runboat and used the existing form button rule for sales orders and so far I am not seeing the button.

So, I was working mostly on the output direction for EDI and I am struggling to understand how to test it manually.
Following my ticket with "Documentation required" I would have liked some update on the readme file, with a section how to setup the new decoupled rules.

But as far as I understand it, I think the change is really good.
For my case, I mostly just have a model in the exchange type rules and let the model listener do the rest so my knowledge ends here.

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Tested locally against l10n-spain tests and all worked properly except for the comment.

Good for me then 😄

edi_oca/migrations/14.0.1.20.0/post-migrate.py Outdated Show resolved Hide resolved
@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from b86c6ed to 61748bb Compare August 21, 2023 13:53
@simahawk
Copy link
Contributor Author

Hey Simone, I said I will also look at it this weekend but at the moment I will leave it to Etobella,

I was checking runboat and used the existing form button rule for sales orders and so far I am not seeing the button.

So, I was working mostly on the output direction for EDI and I am struggling to understand how to test it manually. Following my ticket with "Documentation required" I would have liked some update on the readme file, with a section how to setup the new decoupled rules.

But as far as I understand it, I think the change is really good. For my case, I mostly just have a model in the exchange type rules and let the model listener do the rest so my knowledge ends here.

You are right, thanks for your feedback.
I'm adding a new section in the configuration part.

Long awaited improvement.
Allows to decouple rules by model giving much more flexibility.
Migration steps included.
You can now customize and translate labels and tooltips for EDI automatic buttons.
@simahawk simahawk force-pushed the 14-edi-fmwk-imp-rule branch from 61748bb to ee7a7e8 Compare August 21, 2023 14:05
If there's more than a backend and the exchange type has not a backend set,
a wizard will appear asking to select a backend to be used for the exchange.

In case of "Custom" kind, you'll have to define your own logic to do something.

Choose a reason for hiding this comment

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

Much clearer now, thanks!

@simahawk
Copy link
Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-797-by-simahawk-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e8aefdf into OCA:14.0 Aug 21, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 1c2033f. 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.

6 participants