-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
[17.0][MIG] edi_oca: Migration to 17.0 #47
Conversation
cb62973
to
72d9eee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR of base_edi
has been merged, you can remove it from test-requirements.txt
Thanks @nguyenminhchien for your in depth review. Will adjust everything later today. |
e6d02f2
to
cce2c95
Compare
I have applied the suggested changes, all tests now pass. |
Hello @simahawk , I know you are busy, but once you have some minutes, could you check this PR, I need to migrate everything to 17.0 and edi_oca is the most critical. I also saw there was another PR for 16 "add partner form page". Do I need to update the code? Thank you very much! |
Notify the related record belongs now to the exchange record itself as it makes more sense. It has been renamed for better meaning to '_notify_related_record'.
ACK is now provided by a specific type of exchange and relies on the relation between records of given type.
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
@simahawk what are we doing with this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review
Can you rebase please for the runboat @john-herholz-dt ? |
fce214c
to
b9793f7
Compare
b9793f7
to
d67559c
Compare
I did the rebase, somehow it had now problems with the pre-commit checks, I did another one pre-commit on my side |
the last commit will be include with the migration commit
|
a474549
to
59ec2fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review, LGTM
This PR has the |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Finally! Jeehaaaa. |
Congratulations, your PR was merged at 91b3a7f. Thanks a lot for contributing to OCA. ❤️ |
A lot of views needed to be adjusted.
Furthermore I changed the _search method on edi_exchange_record, as the count param no longer works.
Not sure about the change as I don't know what the override of the _search method was suppossed to do.
Edit: @nguyenminhchien helped on the _search method.