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] sale_order_import_edifact: Migration to 16.0 #821

Merged
merged 13 commits into from
Nov 13, 2023

Conversation

QuocDuong1306
Copy link

@QuocDuong1306 QuocDuong1306 commented Sep 28, 2023

@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from 58d1e64 to 1ccab24 Compare September 28, 2023 10:11
@nilshamerlinck
Copy link
Contributor

Note:

  • sale_order_import_edifact is now just bringing EDIFACT support to sale_order_import, disconnected from the EDI framework
  • it's edi_sale_edifact_oca, similarily to edi_sale_ubl_oca, that will glue sale_order_import_edifact with the EDI framework from now on


@api.onchange("order_file")
def order_file_change(self):
if self.edifact_ok:
Copy link
Contributor

Choose a reason for hiding this comment

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

see my comments on #816

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this. We should leverage the call to super for loading defaults.
Also, here, the doc_type and the price_source are kind of hardcoded...
Shouldn't we rely on the parsing of the file as for the other formats?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to be able to force from configuration.

sale_order_import:
      price_source: order
      confirm_order: false
      wiz_ctx:
         file_ext: 'edi'
         release: 'd96a'
         doc_type: 'rfq'

but yes, price_source should be taken from context like doc_type

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the point. Instead of having special keys, we can rely on the fact that defaults will be taken automatically.
In they yml example above I'd like to get rid of the special keys price_source and confirm_order.
In the wiz ctx we should see something like:

wiz_ctx:
    default_file_ext: edi
    default_release: d96a
    default_doc_type: rfq
    default_price_source: order
    default_confirm_order: true

and if I'm not mistaken the only field missing here is confirm_order which I'd like to add as a boolean in the sale.order.import wizard to automatically confirm it instead for relying on a custom machinery in edi_sale_oca.
TBH is my "fault". Is something that I didn't introduce in v 14 and now I'd like to clear it up.
Hope is clear 😉

Copy link
Author

Choose a reason for hiding this comment

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

I created a PR for confirm_order field in sale_order_import module at #839
Could you have a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

@QuocDuong1306 QuocDuong1306 marked this pull request as draft October 4, 2023 01:29
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from 1ccab24 to bd5f7b1 Compare October 11, 2023 04:37
@QuocDuong1306 QuocDuong1306 marked this pull request as ready for review October 11, 2023 04:40
@QuocDuong1306
Copy link
Author

Hi @simahawk , I updated

sale_order_import_edifact/readme/ROADMAP.rst Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved

@api.onchange("order_file")
def order_file_change(self):
if self.edifact_ok:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this. We should leverage the call to super for loading defaults.
Also, here, the doc_type and the price_source are kind of hardcoded...
Shouldn't we rely on the parsing of the file as for the other formats?

sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
test-requirements.txt Outdated Show resolved Hide resolved
@QuocDuong1306 QuocDuong1306 marked this pull request as draft October 18, 2023 07:01
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch 2 times, most recently from 3a201e2 to 652f5cd Compare October 19, 2023 03:36
@QuocDuong1306
Copy link
Author

I updated, but waiting for your reply to the comments above:

This was an old item that was in the old version, so may I remove it?
This button was added by the original author of the module in v14. So do you want me to remove this button?

After that, I will force push again

@QuocDuong1306 QuocDuong1306 marked this pull request as ready for review October 19, 2023 03:47
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from 652f5cd to dcdcad4 Compare October 19, 2023 04:45
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from dcdcad4 to 17ce326 Compare October 20, 2023 03:59
sale_order_import_edifact/__manifest__.py Outdated Show resolved Hide resolved
_inherit = "business.document.import"

@api.model
def _hook_match_partner(self, partner_dict, chatter_msg, domain, order):
Copy link
Contributor

Choose a reason for hiding this comment

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

matching a partner via gln should be a generic thing... what is really needed here that is specific to edifact?
IMO there's something "wrong" or "too rigid" here

whereas it is assumed that the scheme comes from a UBL file...

If you don't have time to solve it now, I would a TODO to refactor that piece of code and drop this one.

CC @jbaudoux

sale_order_import_edifact/readme/ROADMAP.rst Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from 17ce326 to 901edc5 Compare October 25, 2023 04:17
@QuocDuong1306
Copy link
Author

QuocDuong1306 commented Oct 25, 2023

I updated

@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from 901edc5 to c3ae082 Compare October 25, 2023 07:59
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Almost there! 🦾
Dependencies to drop and small remarks on the doc type.

test-requirements.txt Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
sale_order_import_edifact/wizard/sale_order_import.py Outdated Show resolved Hide resolved
@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch from 1c31f90 to 204efcd Compare October 26, 2023 09:25
@QuocDuong1306
Copy link
Author

OK @simahawk , done here

@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-mig-sale_order_import_edifact branch 2 times, most recently from 3bd9ed1 to c251cc1 Compare October 27, 2023 03:06
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

LG

@simahawk
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 16.0.

@OCA-git-bot OCA-git-bot force-pushed the 16.0-mig-sale_order_import_edifact branch from c251cc1 to a22cf47 Compare November 13, 2023 12:47
@simahawk
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 05af771 into OCA:16.0 Nov 13, 2023
6 checks passed
@OCA-git-bot
Copy link
Contributor

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