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] [IMP] sale_input_barcode: scan directly on sale.order #669

Merged
merged 2 commits into from
Feb 24, 2025
Merged

[14.0] [IMP] sale_input_barcode: scan directly on sale.order #669

merged 2 commits into from
Feb 24, 2025
+113 −11

Conversation

SylweKra
Copy link

@SylweKra SylweKra commented Jan 22, 2025 •
edited
Loading

Superseeds #611

@OCA-git-bot
Copy link
Contributor

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

@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch 2 times, most recently from a3fc16d to 02ddda4 Compare January 22, 2025 14:08
@SylweKra SylweKra mentioned this pull request Jan 22, 2025
Open
aleuffre
aleuffre suggested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Hello, thank you for the PR! Looks good overall, just a couple of minor fixes IMO

sale_input_barcode/readme/USAGE.rst Outdated
Comment on lines 13 to 14
If a product is already inside the sale order lines,
the quantity will be updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: Update documentation

Copy link
Author

Choose a reason for hiding this comment

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

I added the documentation.Let me know if it is clear and understandable.

sale_input_barcode/views/sale_input_settings_view.xml Outdated
Comment on lines 7 to 17
<xpath expr="//div[@id='no_edit_order']" position="inside">
<div class="o_setting_left_pane">
<field name="sale_sol_creation_settings" />
</div>
<div class="o_setting_right_pane">
<label for="sale_sol_creation_settings" />
<div class="text-muted">
Instead of creating a new sale order line it increases the quantity for each barcode scan
</div>
</div>



</xpath>
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: could you fix the formatting here, please?

Copy link
Author

Choose a reason for hiding this comment

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

I have improved the formatting. Let me know.

sale_input_barcode/models/res_config_settings.py Outdated

_inherit = "res.config.settings"

sale_sol_creation_settings = fields.Boolean(
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick(non-blocking): the name of this setting does not clearly reflect what the setting does IMO.

what about something like: sale_barcode_update_existing_line?

Copy link
Author

Choose a reason for hiding this comment

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

ok, I've changed the name of the variable as you suggested.

@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch 2 times, most recently from 1cf4e2b to d1bef11 Compare January 23, 2025 09:00
@SylweKra SylweKra changed the title [14.0] [FIX] sale_input_barcode [14.0] [IMP] sale_input_barcode Jan 23, 2025
@SylweKra SylweKra changed the title [14.0] [IMP] sale_input_barcode [14.0] [IMP] sale_input_barcode: scan directly on sale.order Jan 23, 2025
@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch 3 times, most recently from 6a04e00 to b546cda Compare January 24, 2025 09:15
aleuffre
aleuffre approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, LGTM

sale_input_barcode/models/__init__.py
@@ -1,2 +1,3 @@
from . import product_barcode_line_mixin
from . import sale_order
from . import res_config_settings
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick (non-blocking): unless there are reasons to do otherwise, prefer listing the files in alphabetical order.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can suggest to add isort (and black) to your environment for that.

Note this is not an OCA requirement.

Copy link
Author

Choose a reason for hiding this comment

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

I ordered the files in alphabetical order like you suggested.

@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch from b546cda to b0ed460 Compare January 30, 2025 08:28
@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch from b0ed460 to a52bc2c Compare February 20, 2025 16:08
francesco-ooops
francesco-ooops approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Functional is ok!

@bealdav @rousseldenis do you think it's good for merge? Thanks!

@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). 🤖

@francesco-ooops
Copy link
Contributor

@bealdav @OCA/logistics-maintainers when possible, thanks 🙏

jbaudoux
jbaudoux approved these changes Feb 24, 2025
View reviewed changes
Copy link

@jbaudoux jbaudoux 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 drop generated files from IMP/FIX commits?

You can use export SKIP=oca-gen-addon-readme

@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch 2 times, most recently from 96cb892 to 743b095 Compare February 24, 2025 13:27
@SylweKra SylweKra force-pushed the 14.0-FIX-sale_input_barcode branch from 743b095 to 9680ac5 Compare February 24, 2025 13:29
@jbaudoux
Copy link

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-669-by-jbaudoux-bump-minor, awaiting test results.

@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 99d6f5b. Thanks a lot for contributing to OCA. ❤️

@OCA-git-bot OCA-git-bot merged commit 7463611 into OCA:14.0 Feb 24, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousseldenis rousseldenis rousseldenis left review comments

@jbaudoux jbaudoux jbaudoux approved these changes

@francesco-ooops francesco-ooops francesco-ooops approved these changes

@aleuffre aleuffre aleuffre approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@SylweKra @OCA-git-bot @francesco-ooops @jbaudoux @rousseldenis @aleuffre @renda-dev