OCA / stock-logistics-barcode

https://odoo-community.org/psc-teams/logistics-18
GNU Affero General Public License v3.0
158 stars 324 forks source link

[14.0] [ADD] barcode_generator_product_variant #575

Closed renda-dev closed 7 months ago

renda-dev commented 7 months ago

Generate missing barcodes for a product's variants in bulk.

renda-dev commented 7 months ago

Thank you for the huge review! @legalsylvain

I've modified this PR according to the majority of the things that you pointed out, please let me know what you think :)

francesco-ooops commented 7 months ago

@legalsylvain ping :)

legalsylvain commented 7 months ago

Hi. Thanks for all the changes !

I mean, it doesn't impact much on performance and it's a great feature in my opinion

Well, I just say that setting a default on a field is a native feature in odoo. So no need to add dozens of lines of code for that. Don't you think ?

image

francesco-ooops commented 7 months ago

Hi. Thanks for all the changes !

I mean, it doesn't impact much on performance and it's a great feature in my opinion

Well, I just say that setting a default on a field is a native feature in odoo. So no need to add dozens of lines of code for that. Don't you think ?

image

Hi @legalsylvain , this feature you're referring to it's hardly user-friendly, requires developer mode access and is often not working as expected. A company with hundreds of employees cannot really rely on setting a default value for each user.

On the contrary, it's quite common to add a default field in settings to specify a default value company-wide, but in this case there is no dependency on a specific app where we can add this "default" field, so we resorted to adding it on the record itself.

I second @renda-dev opinion that it shouldn't be a big deal, so if it's not blocking we'd appreciate the approval :)

legalsylvain commented 7 months ago

Hi @legalsylvain , this feature you're referring to it's hardly user-friendly, requires developer mode access and is often not working as expected. A company with hundreds of employees cannot really rely on setting a default value for each user.

Well, for me :

this feature you're referring to it's hardly user-friendly, requires developer mode access

create and configure barcode rule is really not user-friendly. It has to be done by admin people.

and is often not working as expected.

could you elaborate ?

A company with hundreds of employees cannot really rely on setting a default value for each user.

The screenshot I did is not correct, I wanted to mention "all users" option. That way the admin instead of clicking on the new boolean, configure one time only, the ir.default. Not more work, just another way to configre.

That being said, it is not a blocking point ! You can go ahead if you think it's the good design.

regards.

francesco-ooops commented 7 months ago

and is often not working as expected.

could you elaborate ?

@legalsylvain I haven't used it lately, but seems to me not all fields can be set as default:

image

The screenshot I did is not correct, I wanted to mention "all users" option. That way the admin instead of clicking on the new boolean, configure one time only, the ir.default. Not more work, just another way to configre.

I couldn't find a way to do that. Can you manage in runboat to set one barcode rule to be used in wizard using your suggestion? Thanks!

aleuffre commented 7 months ago

and is often not working as expected.

could you elaborate ?

@legalsylvain I haven't used it lately, but seems to me not all fields can be set as default:

image

The screenshot I did is not correct, I wanted to mention "all users" option. That way the admin instead of clicking on the new boolean, configure one time only, the ir.default. Not more work, just another way to configre.

I couldn't find a way to do that. Can you manage in runboat to set one barcode rule to be used in wizard using your suggestion? Thanks!

You are trying to set the default on the product model, instead of the wizard model.

  1. Click "Manage missing barcodes"
  2. Click the debug symbol on the top left of the popup

image

  1. Click "Set Default" there.
francesco-ooops commented 7 months ago

Ok, this is good to know for someone who implements, but terrible UX if you want a responsible user (not "any" user) to be independent in setting the default barcode rule IMHO. I would maintain the current implementation.

OCA-git-bot commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@legalsylvain could I ask for approval? :)

legalsylvain commented 7 months ago

Hi @francesco-ooops. the PR is still approved, and I have no merge right on that repo. (https://github.com/OCA/repo-maintainer-conf/blob/master/conf/psc/logistics.yml)

thanks.

francesco-ooops commented 7 months ago

sure, I didn't ask for merging, just for the tick as you reviewed the module, but no biggie :)

image

renda-dev commented 7 months ago

Hi, I've moved the "Default Rule" option in the barcode_generator_abstract module, and I personally think that it's a great feature (299b11cdd54120dcb56b14f970e7eff5bb76f7a9)

Please, let me know what do you think about that :) @legalsylvain @rousseldenis

OCA-git-bot commented 7 months ago

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 commented 7 months ago

@rousseldenis good to go? :)

rousseldenis commented 7 months ago

/ocabot merge patch

OCA-git-bot commented 7 months ago

Hey, thanks for contributing! Proceeding to merge this for you. Prepared branch 14.0-ocabot-merge-pr-575-by-rousseldenis-bump-patch, awaiting test results.

OCA-git-bot commented 7 months ago

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