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][FIX]pos_order_remove_line: Action on trash delete line #1120

Closed
wants to merge 1 commit into from

Conversation

DorianMAG
Copy link
Contributor

Change javascript method to delete a line when click on the trash

@OCA-git-bot
Copy link
Contributor

Hi @robyf70,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Lgtm.

Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

LGTM

@flotho
Copy link
Member

flotho commented Jan 5, 2024

pop @DorianMAG please correct the pre commit

@DorianMAG
Copy link
Contributor Author

pop @DorianMAG please correct the pre commit

Dont understand why pre-commit failed.
Xml file look good

@DorianMAG
Copy link
Contributor Author

pop @DorianMAG please correct the pre commit

Finally understand, semicolon less in javascript file.
Thx for the help
Regards

Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

this module is not concerned by the test failure.
LGTM

Copy link
Contributor

@robyf70 robyf70 left a comment

Choose a reason for hiding this comment

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

LGTM

@robyf70
Copy link
Contributor

robyf70 commented Jan 5, 2024

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1120-by-robyf70-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 5, 2024
Signed-off-by robyf70
@OCA-git-bot
Copy link
Contributor

@robyf70 your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1120-by-robyf70-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@DorianMAG
Copy link
Contributor Author

@robyf70
Odoo test failed on pos_order_to_sale_order module.
Any idea to fix the problem?
Regards

@robyf70
Copy link
Contributor

robyf70 commented Jan 8, 2024

@robyf70 Odoo test failed on pos_order_to_sale_order module. Any idea to fix the problem? Regards

No idea how to fix it

@DorianMAG
Copy link
Contributor Author

@robyf70
All tests passed in PR.
Can you retry the merge plz.
Regards

@robyf70
Copy link
Contributor

robyf70 commented Jan 8, 2024

@robyf70 Odoo test failed on pos_order_to_sale_order module. Any idea to fix the problem? Regards

@legalsylvain Can you please look at this error?

2024-01-05 09:04:36,584 339 ERROR odoo odoo.addons.pos_order_to_sale_order.tests.test_module: FAIL: TestUi.test_pos_order_to_sale_order
Traceback (most recent call last):
  File "/__w/pos/pos/pos_order_to_sale_order/tests/test_module.py", line 36, in test_pos_order_to_sale_order
    self.assertEqual(len(before_orders) + 1, len(after_orders))
AssertionError: 1 != 0

@robyf70
Copy link
Contributor

robyf70 commented Jan 8, 2024

/ocabot merge minor

Let's try

@robyf70
Copy link
Contributor

robyf70 commented Jan 8, 2024

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1120-by-robyf70-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 8, 2024
Signed-off-by robyf70
@legalsylvain
Copy link
Contributor

legalsylvain commented Jan 8, 2024

@legalsylvain Can you please look at this error?

No time to investigate. indeed, something look broken in V16 branch. however, I just tested pos_order_to_sale_order solo, and it works. So I guess that it is an incompatibility with other module(s)
I can take a look in a few weeks.

@OCA-git-bot
Copy link
Contributor

@robyf70 your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1120-by-robyf70-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

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.

This shouldn't be merged as it is:

  • The commits should be squashed into a single one
  • The commit message should have a proper description, that contains a tag, the module name and the essence of the change e.g.: "[FIX] pos_order_remove_line: trash action"
  • There are unnecessary changes in spacings

@DorianMAG
Copy link
Contributor Author

@robyf70 Odoo test failed on pos_order_to_sale_order module. Any idea to fix the problem? Regards

@legalsylvain Can you please look at this error?

2024-01-05 09:04:36,584 339 ERROR odoo odoo.addons.pos_order_to_sale_order.tests.test_module: FAIL: TestUi.test_pos_order_to_sale_order
Traceback (most recent call last):
  File "/__w/pos/pos/pos_order_to_sale_order/tests/test_module.py", line 36, in test_pos_order_to_sale_order
    self.assertEqual(len(before_orders) + 1, len(after_orders))
AssertionError: 1 != 0

Hi @legalsylvain
Thx, i take a look

@DorianMAG
Copy link
Contributor Author

This shouldn't be merged as it is:

* The commits should be squashed into a single one

* The commit message should have a proper description, that contains a tag, the module name and the essence of the change e.g.: "[FIX] pos_order_remove_line: trash action"

* There are unnecessary changes in spacings

Thx, i take a look for commit recomandation.
spacings changes was come from pre-commit, the original code did not have the correct indent

@DorianMAG
Copy link
Contributor Author

This shouldn't be merged as it is:

* The commits should be squashed into a single one

* The commit message should have a proper description, that contains a tag, the module name and the essence of the change e.g.: "[FIX] pos_order_remove_line: trash action"

* There are unnecessary changes in spacings

It's done,
Regards

Copy link

@JulienMartinez JulienMartinez left a comment

Choose a reason for hiding this comment

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

LGTM

@DorianMAG DorianMAG changed the title Change javascript method to delete a line when click on the trash [16.0][FIX]Change javascript method to delete a line when click on the trash Jan 8, 2024
@DorianMAG DorianMAG changed the title [16.0][FIX]Change javascript method to delete a line when click on the trash [16.0][FIX]Action on trash delete line Jan 8, 2024
@DorianMAG DorianMAG changed the title [16.0][FIX]Action on trash delete line [16.0][FIX]pos_order_remove_line: Action on trash delete line Jan 8, 2024
@danielduqma
Copy link
Contributor

Hi @DorianMAG,

These changes revert some fixes made in #1044. I checked it on runboat, having pos_loyalty installed:

  1. Select a customer
  2. Add a product to cart
  3. Add another different product to cart
  4. Remove last line
  5. Loyalty points balance hasn't changed

cc @papulo79

@papulo79
Copy link

papulo79 commented Jan 9, 2024

Hi @DorianMAG,

I've repeat the process described by @danielduqma and I had the same results.

This MR has broken the module with pos loyalty.

@robyf70

@DorianMAG
Copy link
Contributor Author

@papulo79 @danielduqma
Manifestly, the entire order is updated, except loyalty.
I will take a look

@ivantodorovich
Copy link
Contributor

@DorianMAG I take from the last comments that this PR is still not ready yet?

@DorianMAG
Copy link
Contributor Author

@DorianMAG I take from the last comments that this PR is still not ready yet?

Hi,
indeed, i need to find a solution.
I try to find time this week to fix that.

but this one is ok
#1122

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 Jul 14, 2024
@github-actions github-actions bot closed this Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs fixing stale PR/Issue without recent activity, it'll be soon closed automatically. work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants