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

[16.0][IMP] pos_order_to_sale_order : use customer note and correctly prepare sale.order.line name fields #1080

Merged
merged 3 commits into from
Nov 24, 2023
Merged

[16.0][IMP] pos_order_to_sale_order : use customer note and correctly prepare sale.order.line name fields #1080

merged 3 commits into from
Nov 24, 2023

Conversation

legalsylvain
Copy link
Contributor

@legalsylvain legalsylvain commented Nov 10, 2023 •
edited
Loading

context : when using pos_order_to_sale_order in V16 :

  • the name of the sale.order.line is not like when we create sale order from UI. (the default_code is not used, the sale_description is not used...)
  • On the other hand, if cashier set a customer note in the PoS, the value is lost.

This PR fixes both problem.

Supersed : #1065

Previous implementation :

image

New implementation :

image

@legalsylvain
[REF] pos_order_to_sale_order : add sale order line sequence in the p…
e8967d2 
…repare function
@legalsylvain
[IMP] pos_order_to_sale_order : use customer_note to prepare sale ord…
2b18895 
…er line name fields
@legalsylvain
[IMP] pos_order_to_sale_order : performance : directly overload _get_…
85cd913 
…sale_order_line_multiline_description_sale to avoid to write on sale.order.line after having creating the sale.order.line
@legalsylvain legalsylvain added this to the 16.0 milestone Nov 10, 2023
@legalsylvain legalsylvain mentioned this pull request Nov 10, 2023
Closed
@legalsylvain
Copy link
Contributor Author

Hi @ivs-cetmix, @geomer198 . could you take a look on this one ?

ivs-cetmix
ivs-cetmix approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

LGTM

@ivs-cetmix
Copy link
Member

Hi @ivs-cetmix, @geomer198 . could you take a look on this one ?

@legalsylvain Looks good! Thank you!

geomer198
geomer198 reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

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

Could you fix my comments and add tests for this implementation?

pos_order_to_sale_order/models/sale_order.py
Command.create(SaleOrderLine._prepare_from_pos(line[2]))
for line in order_data["lines"]
Command.create(SaleOrderLine._prepare_from_pos(i + 1, line_data[2]))
for (i, line_data) in enumerate(order_data["lines"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think need add a start argument for starting from 1.

 Command.create(SaleOrderLine._prepare_from_pos(i, line_data[2]))
 for (i, line_data) in enumerate(order_data["lines"], start=1)

pos_order_to_sale_order/models/sale_order_line.py
res = super()._get_sale_order_line_multiline_description_sale()

for (i, line_data) in enumerate(
self.env.context.get("pos_order_lines_data", [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

pos_order_to_sale_order/models/sale_order_line.py
self.env.context.get("pos_order_lines_data", [])
):
if line_data.get("customer_note", False) and self.sequence == i + 1:
res += "\n" + line_data.get("customer_note")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here better use f-string:

res += f"\n{line_data.get('customer_note')}"

@legalsylvain
Copy link
Contributor Author

Could you fix my comments and add tests for this implementation?

No time in the next days. Could you make a PR against my branch ?
I'll review it and merge it quickly. Thanks !

@ivs-cetmix
Copy link
Member

Could you fix my comments and add tests for this implementation?

No time in the next days. Could you make a PR against my branch ? I'll review it and merge it quickly. Thanks !

@legalsylvain sure, thank you! Will do

@geomer198
Copy link
Contributor

Could you fix my comments and add tests for this implementation?

No time in the next days. Could you make a PR against my branch ? I'll review it and merge it quickly. Thanks !

Could you get me an access to creating PR? I got an error 403 at creating PR in your branch.

@legalsylvain
Copy link
Contributor Author

Could you get me an access to creating PR? I got an error 403 at creating PR in your branch.

I don't understand. You should have write to create a PR. could you say me where is your branch ?

@geomer198
Copy link
Contributor

geomer198 commented Nov 16, 2023 •
edited by legalsylvain
Loading

Could you get me an access to creating PR? I got an error 403 at creating PR in your branch.

I don't understand. You should have right to create a PR. could you say me where is your branch ?

I created branch from this branch. I cloned branch in local and created a new brench from your.

@legalsylvain
Copy link
Contributor Author

I created branch from this branch. I cloned branch in local and created a new brench from your.

yes, but where is your branch on github ?

@geomer198
Copy link
Contributor

I created branch from this branch. I cloned branch in local and created a new brench from your.

yes, but where is your branch on github ?

I can't create branch on Github and get an error:

fatal: unable to access 'https://github.com/grap/pos.git/': The requested URL returned error: 403

@legalsylvain
Copy link
Contributor Author

You have to push your branch on a remote you have write access.

@geomer198
Copy link
Contributor

You have to push your branch on a remote you have write access.

Sorry, I don't understand what you mean. I try push to your remote repository and doesn't access to your repository.

@cvinh
Copy link
Contributor

cvinh commented Nov 18, 2023 •
edited
Loading

You have to push your branch on a remote you have write access.

Sorry, I don't understand what you mean. I try push to your remote repository and doesn't access to your repository.

1- fork the repo
2- create a new branch to your fork
3- push your commit to your fork
4- do a PR from your fork towards the repo

@geomer198
Copy link
Contributor

1- fork this repo
2- create a new branch to your fork
3- push your commit to your fork
4- do a PR from your fork towards this repo

Thanks a lot!

@geomer198
Copy link
Contributor

You have to push your branch on a remote you have write access.

Done! grap#4

@geomer198 geomer198 mentioned this pull request Nov 18, 2023
Closed
@legalsylvain
Copy link
Contributor Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1080-by-legalsylvain-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 81442c8 into OCA:16.0 Nov 24, 2023
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@geomer198 geomer198 geomer198 left review comments

@ivs-cetmix ivs-cetmix ivs-cetmix approved these changes

Assignees
No one assigned
Labels
merged 🎉
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
5 participants
@legalsylvain @ivs-cetmix @geomer198 @cvinh @OCA-git-bot