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] [MIG] pos_order_remove_line: migration to 16.0 #925

Merged
merged 8 commits into from
Feb 20, 2023
Merged

[16.0] [MIG] pos_order_remove_line: migration to 16.0 #925

merged 8 commits into from
Feb 20, 2023

Conversation

suker
Copy link
Contributor

@suker suker commented Feb 13, 2023

PR for migration from 14.0 to 16.0

@suker
Copy link
Contributor Author

suker commented Feb 13, 2023

Related task: #849

@suker suker changed the title 16.0 mig pos order remove line [16.0] [MIG] pos order remove line: migration to 16.0 Feb 13, 2023
@suker suker changed the title [16.0] [MIG] pos order remove line: migration to 16.0 [16.0] [MIG] pos_order_remove_line: migration to 16.0 Feb 13, 2023
@suker suker mentioned this pull request Feb 13, 2023
Open
37 tasks
@legalsylvain

This comment was marked as off-topic.

Sign in to view
@OCA-git-bot

This comment was marked as off-topic.

Sign in to view
@legalsylvain
Copy link
Contributor

/ocabot migration pos_order_remove_line

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Feb 13, 2023
@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 474760c to 4b39c83 Compare February 14, 2023 10:50
chienandalu
chienandalu requested changes Feb 14, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Check and fix pre-commit errors and warnings

@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch 2 times, most recently from 47382c0 to 746016d Compare February 14, 2023 11:02
@suker
Copy link
Contributor Author

suker commented Feb 14, 2023

Check and fix pre-commit errors and warnings

Yup I think It doesn't like to use import and export in .js files...

@chienandalu
Copy link
Member

Check and fix pre-commit errors and warnings

Yup I think It doesn't like to use import and export in .js files...

When using ES6 you have to rename the files to .esm.js

@suker
Copy link
Contributor Author

suker commented Feb 14, 2023

Check and fix pre-commit errors and warnings

Yup I think It doesn't like to use import and export in .js files...

When using ES6 you have to rename the files to .esm.js

That's true...
I saw it in migration to v15.0 guidelines...
image

Gracias 👍

flachica
flachica reviewed Feb 14, 2023
View reviewed changes
Copy link
Contributor

@flachica flachica left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If delete the last line of ticket, exception is launched because event pos_restaurant.Orderline.selectLine arrived

@suker
Copy link
Contributor Author

suker commented Feb 15, 2023

Check and fix pre-commit errors and warnings

If delete the last line of ticket, exception is launched because event pos_restaurant.Orderline.selectLine arrived

What do you mind?

@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 746016d to 280137d Compare February 15, 2023 15:49
@suker
Copy link
Contributor Author

suker commented Feb 15, 2023

Check and fix pre-commit errors and warnings

pre-commit is failing if we do not update version of isort to 12.0

image

is this error related?

@ivantodorovich
Copy link
Contributor

Should be fixed here:

@ivantodorovich
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

@ivantodorovich The rebase process failed, because command git push --force factorlibre tmp-pr-925:16.0-mig-pos_order_remove_line failed with output:

remote: Permission to factorlibre/pos.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/factorlibre/pos/': The requested URL returned error: 403

@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 280137d to 498aaa6 Compare February 16, 2023 07:56
chienandalu
chienandalu reviewed Feb 16, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Can you check @flachica comment?

pos_order_remove_line/static/src/js/orderline.js Outdated Show resolved Hide resolved
@flachica
Copy link
Contributor

Check and fix pre-commit errors and warnings

If delete the last line of ticket, exception is launched because event pos_restaurant.Orderline.selectLine arrived

What do you mind?

What I mean is that if:

1 I add a single line to the ticket
2 I use the trash button to delete the only line of the ticket that is there

The POS throws an exception

image

@GabbasovDinar GabbasovDinar mentioned this pull request Feb 16, 2023
Closed
@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 498aaa6 to 161c94c Compare February 16, 2023 18:00
@suker
Copy link
Contributor Author

suker commented Feb 16, 2023

Check and fix pre-commit errors and warnings

If delete the last line of ticket, exception is launched because event pos_restaurant.Orderline.selectLine arrived

What do you mind?

What I mean is that if:

1 I add a single line to the ticket 2 I use the trash button to delete the only line of the ticket that is there

The POS throws an exception

image

I see... but that error is throwed by pos_restaurant module, It should be fixed there

@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 161c94c to 868eada Compare February 16, 2023 18:07
@flachica
Copy link
Contributor

IMHO I think the bug is in this development. The pos_restaurant module is maintained by Odoo SA and without this module it would not fail. I am not yet knowledgeable enough to suggest you the necessary modification. I am investigating and as soon as I can I will make the suggestion. Regards

@chienandalu
Copy link
Member

Taking a quick glance at the code it looks to me that a glue module would be needed to make it compatible with pos_restaurant returning to super in case there wouldn't be a line id: https://github.com/odoo/odoo/blob/880afb3006ece93ee6575c977233bfb61081bf3a/addons/pos_restaurant/static/src/js/Screens/ProductScreen/Orderline.js#L27-L39

It can go to the Roadmap section in any case, as not every instance use the restaurant features.

@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 868eada to 4f8b39d Compare February 17, 2023 12:34
@suker
Copy link
Contributor Author

suker commented Feb 17, 2023

IMHO I think the bug is in this development. The pos_restaurant module is maintained by Odoo SA and without this module it would not fail. I am not yet knowledgeable enough to suggest you the necessary modification. I am investigating and as soon as I can I will make the suggestion. Regards

Thanks you for your opinion, after that I did some modifications to avoid the error.
It also deletes reward lines when loyalty programs are active.

@suker
Copy link
Contributor Author

suker commented Feb 17, 2023 •
edited
Loading

Taking a quick glance at the code it looks to me that a glue module would be needed to make it compatible with pos_restaurant returning to super in case there wouldn't be a line id: https://github.com/odoo/odoo/blob/880afb3006ece93ee6575c977233bfb61081bf3a/addons/pos_restaurant/static/src/js/Screens/ProductScreen/Orderline.js#L27-L39

It can go to the Roadmap section in any case, as not every instance use the restaurant features.

I found another way to avoid the error. It's already pushed, can you take a look?

Thanks 👍

@flachica
Copy link
Contributor

flachica commented Feb 17, 2023 •
edited
Loading

Tested and working. Thank you very much

@suker suker force-pushed the 16.0-mig-pos_order_remove_line branch from 4f8b39d to afd325a Compare February 20, 2023 07:53
chienandalu
chienandalu approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thanks :)

@chienandalu
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 224c00e into OCA:16.0 Feb 20, 2023
@OCA-git-bot
Copy link
Contributor

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

@flachica flachica flachica approved these changes

@chienandalu chienandalu chienandalu approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
8 participants
@suker @legalsylvain @OCA-git-bot @chienandalu @ivantodorovich @flachica @robyf70 @fshah-initos