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

[16.0][MIG] edi_oca #5

Merged
merged 11 commits into from
Oct 2, 2023
Merged

[16.0][MIG] edi_oca #5

merged 11 commits into from
Oct 2, 2023

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Jun 1, 2023

@john-herholz-dt
Copy link
Contributor

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

EvaSForgeFlow added a commit to ForgeFlow/edi-framework that referenced this pull request Jun 9, 2023
@john-herholz-dt
Copy link
Contributor

@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
Copy link
Member

@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 simahawk force-pushed the 16-edi_oca branch 2 times, most recently from 620389e to 257b708 Compare August 12, 2023 10:20
@simahawk
Copy link
Contributor Author

simahawk commented Aug 12, 2023

I've found out that the tests classes that were causing infinite hanging were the ones using ComponentRegistryCase.
Here's the fix OCA/connector#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 OCA/edi#797 is integrated (BTW a review there would be welcomed 😉)

@simahawk simahawk marked this pull request as ready for review August 12, 2023 10:23
@johnny-longneck
Copy link

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
Copy link
Contributor

john-herholz-dt commented Aug 16, 2023

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
Copy link
Contributor

john-herholz-dt commented Aug 21, 2023

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
Copy link
Contributor Author

Not yet, I have to port OCA/edi#797

@simahawk
Copy link
Contributor Author

simahawk commented Sep 9, 2023

Fwd ported everything from v14 and made module unistallable here #16
oca-port blacklist added too, so that we don't bother about ported PRs anymore.

@simahawk
Copy link
Contributor Author

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

@simahawk
Copy link
Contributor Author

simahawk commented Sep 11, 2023

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

@john-herholz-dt
Copy link
Contributor

john-herholz-dt commented Sep 11, 2023

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
Copy link
Contributor Author

simahawk commented Sep 12, 2023

@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 😉
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
Copy link
Contributor

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_account_oca 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
Copy link
Contributor

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

@simahawk
Copy link
Contributor Author

simahawk commented Oct 2, 2023

@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
Copy link
Contributor

nilshamerlinck commented Oct 2, 2023

Hi @simahawk small fix here for this PR: camptocamp#1

this aside, this PR worked fine on our side 👍

@simahawk
Copy link
Contributor Author

simahawk commented Oct 2, 2023

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

simahawk and others added 3 commits October 2, 2023 16:09
TypeError: not all arguments converted during string formatting
AccessError core messages were modified
@simahawk
Copy link
Contributor Author

simahawk commented Oct 2, 2023

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

@john-herholz-dt
Copy link
Contributor

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

@simahawk
Copy link
Contributor Author

simahawk commented Oct 2, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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 OCA-git-bot merged commit adb4014 into OCA:16.0 Oct 2, 2023
6 checks passed
@OCA-git-bot
Copy link
Contributor

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

7 participants