OCA / edi-framework

GNU Affero General Public License v3.0
6 stars 29 forks source link

[16.0][MIG] edi_oca #5

Closed simahawk closed 11 months ago

simahawk commented 1 year ago

~Trying to understand what's wrong on #1 w/ tests~ Replaces #1

Requires

john-herholz-dt commented 1 year ago

Any progress? Anybody has a local runboat setup and can debug it?

john-herholz-dt commented 1 year ago

@simahawk I am on this again and found out that there is something wrong on edi_exchange_consumer_mixin

There was an override for the method fields_view_get, since it is deprecated for Odoo 16.0 I tried to rewrite it for the method view_get

Unfortunatly, the result is not as expected. You can check this for example when using edi_purchase_oca, too and then open a purchase.order form view (all fields are visible)

Maybe this is what is breaking the tests on runbot? The fields_view_get stuff exceeds my current Odoo knowledge and I think I cannot solve it. Can you have a look at this?

In case @etobella was the author of the fields_view_get implementation, maybe he can help?

Thank you! John

etobella commented 1 year ago

@john-herholz-dt It was something I implemented.

There is a similar approach on base_tier_validation. You can check how they solved it here:

https://github.com/OCA/server-ux/blob/16.0/base_tier_validation/models/tier_validation.py#L466-L517

simahawk commented 1 year ago

I've found out that the tests classes that were causing infinite hanging were the ones using ComponentRegistryCase. Here's the fix https://github.com/OCA/connector/pull/466

@john-herholz-dt I'll keep only this PR opened. I cleaned up and squashed your commits, added a few more (some to be backported).

Plus, I don't want to merge this till https://github.com/OCA/edi/pull/797 is integrated (BTW a review there would be welcomed :wink:)

johnny-longneck commented 1 year ago

I saw you were working on it yesterday. I also had the RegistryCase as the cause of the timeout in mind, but didn't have time to work on it.

Great you could solve it!!!

I am open to help contributing (maybe with the GS-1 Standard implementation?) since I use the Framework now, and I really like it.

john-herholz-dt commented 1 year ago

I will have a look at OCA/edi#797 this weekend.

The comment above here is also me by the way - just my private account - so don't be confused.

john-herholz-dt commented 1 year ago

This is basically ready, right? Could you give me a hint how to find out when something is wrong on runboat?

I have another PR in a different repo and my test case fails on runboat but succeeds locally - I am struggling to find the proplem there, too.

Thanks. Looking forward to getting this PR merged and have the module finally on 16.0!

simahawk commented 1 year ago

Not yet, I have to port https://github.com/OCA/edi/pull/797

simahawk commented 1 year ago

Fwd ported everything from v14 and made module unistallable here https://github.com/OCA/edi-framework/pull/16 oca-port blacklist added too, so that we don't bother about ported PRs anymore.

simahawk commented 12 months ago

Cleaned up all commits, cleaned up all TODOs for migrations, including rename the disable_edi_auto column.

simahawk commented 12 months ago

@john-herholz-dt can you please do some testing if/when you have time? @nilshamerlinck ready to be tested :wink:

john-herholz-dt commented 12 months ago

I have tested it today on an existing EDI setup with my old migration of the edi_oca module. Everything works, but I had to change the listeners on the models for the EDI creation - to adapt to the new model rules. That was expected. So everything is fine from my side.

EDIT: My EDI model consuming the edi_exchange_consumer_mixing has all fields visible in the form field now. This is due to the get_view method there. I already fixed it in the PR#1 of this repository, you can use the code freely.

image

simahawk commented 12 months ago

@john-herholz-dt thaaaank you very much for you review/test. Much appreciated! In fact, we'd need this kind of help on PRs more often :wink: TBH I missed that change, I wanted to port all relevant changes from you and squash them. I'll check. OTOH it makes me realize we don't have test coverage for this: I'm glad I forgot to pick it -> I'll add a test to get bit again on the next migration.

OriolMForgeFlow commented 12 months ago

Hi @simahawk, Could you please check if the model edi_exchange_consumer_mixin also needs to be migrated? I am currently working on the migration of _edi_accountoca and encountering the following error when I attempt to open the Invoicing app in my local environment. I am not sure if the error is related to the edi_configuration widget:

image

john-herholz-dt commented 11 months ago

I have seen the error above with edi_config also a couple of times.

simahawk commented 11 months ago

@OriolMForgeFlow @john-herholz-dt can you provide me a little bit more ctx? Is it only for migrated instances?

AFAIK nothing has changed on the edi_config field. The error above is not really clear. Could you at least enable ?debug=assets do that we see where is coming from?

In any case, if you don't mind, I would like to merge this as many PRs are piling up waiting for it. We can tackle that problem in another round if it comes up again.

nilshamerlinck commented 11 months ago

Hi @simahawk small fix here for this PR: https://github.com/camptocamp/edi-framework/pull/1

this aside, this PR worked fine on our side :+1:

simahawk commented 11 months ago

I'll check for the tests, has been fixed on 14.0 already but I'd like to improve the fix

simahawk commented 11 months ago

I've just spotted the pre-migrate script was misplaced inside migrations folder :sweat:

john-herholz-dt commented 11 months ago

Nice you are working on it again! Looking forward to having the migration done, hopefully soon!

simahawk commented 11 months ago

/ocabot merge nobump

OCA-git-bot commented 11 months ago

This PR looks fantastic, let's merge it! Prepared branch 16.0-ocabot-merge-pr-5-by-simahawk-bump-nobump, awaiting test results.

OCA-git-bot commented 11 months ago

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