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.

Sign up for GitHub

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

Jump to bottom

[14.0][ADD] sale_order_import_edifact & base_edifact #779

Closed
wants to merge 2 commits into from
Closed

[14.0][ADD] sale_order_import_edifact & base_edifact #779

wants to merge 2 commits into from

Conversation

rmorant
Copy link
Contributor

@rmorant rmorant commented Jun 5, 2023

Plugs EDIFACT/D96A ORDER in sale_order_import process.

@rmorant
Copy link
Contributor Author

rmorant commented Jun 5, 2023

Hi @etobella, no tests yet.

@rmorant
Copy link
Contributor Author

rmorant commented Jun 13, 2023

Tests added in the OCA Days. I don't know if refactoring will be needed with the edi_sale_oca change (#759), but I would mege it now and adapt it when necessary.
What do you think? ( Enric @etobella )

@rmorant rmorant force-pushed the 14.0-mig-sale_order_import_edifact branch from 49a1c7b to 94d5f9e Compare June 28, 2023 11:18
@QuocDuong1306 QuocDuong1306 mentioned this pull request Sep 12, 2023
Merged
@QuocDuong1306 QuocDuong1306 mentioned this pull request Sep 28, 2023
Merged
@simahawk
Copy link
Contributor

simahawk commented Oct 3, 2023

@rmorant question: where is this module coming from? The history seems be incomplete...

@rmorant
Copy link
Contributor Author

rmorant commented Oct 4, 2023

@rmorant question: where is this module coming from? The history seems be incomplete...
Hi Simone,
I only found a PR in v12: https://github.com/PlanetaTIC/edi/tree/12.0-add-base_edifact
I migrated these module, but it did not parse the EDIFACT/D96A files correctly and I created a new one based on the pydifact library.
It has been running in production with Amazon orders for over 6 months, but may not be fully in tune with the latest changes of the framework.
I would like to help in this regard.

@simahawk
Copy link
Contributor

simahawk commented Oct 5, 2023

Ok, then this is not a migration but and addition in fact.
Could you please adapt the description accordingly?
Also, a rebase would be nice so that we can move on (with v16 too).

@simahawk simahawk changed the title [14.0][MIG] sale_order_import_edifact & base_edifact [14.0][ADD] sale_order_import_edifact & base_edifact Oct 5, 2023
simahawk
simahawk requested changes Oct 5, 2023
View reviewed changes
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.

by looking at the current state is probably easier if we move on v16 first (where we are already applying fixes etc) and then backport changes here.
See #813

base_edifact/__manifest__.py
"bin": [],
},
"depends": [
# "edi_party_data_oca"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# "edi_party_data_oca"

base_edifact/models/edifact.py
return obj

@api.model
def get_party_data(self, exchange_record, partner, raise_if_not_found=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

this must be dropped

sale_order_import_edifact/__manifest__.py
"partner_identification",
"partner_identification_gln",
"base_edifact",
"edi_sale_order_import",
Copy link
Contributor

Choose a reason for hiding this comment

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

this module is not meant to plug into edi framework

@rmorant rmorant mentioned this pull request Nov 2, 2023
Closed
@rmorant
Copy link
Contributor Author

rmorant commented Nov 2, 2023

Hi @simahawk, now that you have approved #821. I've been looking at how to backport the modules as you suggested above.
base_edifact does not need changes and i have created the PR: #847
But backporting #821 (sale_order_import_edifact) require changes to the sale_order_import module as those in v16.
What do you think is the best path?
Try to backporting sale_order_import too or add the changes applied in the migration to this one?

@simahawk
Copy link
Contributor

simahawk commented Nov 4, 2023

Try to backporting sale_order_import too or add the changes applied in the migration to this one?

I'd go for this as long as we can do it w/o introducing breaking changes.

@rmorant rmorant mentioned this pull request Nov 9, 2023
Closed
@github-actions GitHub Actions
Copy link

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Mar 10, 2024
@github-actions github-actions bot closed this Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simahawk simahawk simahawk requested changes

Assignees
No one assigned
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmorant @simahawk