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][MIG] stock_picking_auto_create_lot_qty #1567

Open
wants to merge 3 commits into
base: 16.0
Choose a base branch
from

Conversation

dessanhemrayev
Copy link

@dessanhemrayev dessanhemrayev commented Apr 10, 2024

Migration to 16.0

@rousseldenis
Copy link
Contributor

/ocabot migration stock_picking_auto_create_lot_qty

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Apr 11, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Apr 11, 2024
70 tasks
@rousseldenis
Copy link
Contributor

@dessanhemrayev Thanks for this. Could you point to the depending PR(s) in this one description in order to make review easier ?

Copy link

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

Steps to reproduce

Add a few products to the purchase order for instance Line 1: Whiskey qty: 5 and Line 2: Beer qty: 5
Create a picking
Validate a picking

Expected behaviour

Lot's are split for both products and serial numbers assigned

Current behaviour

The beer quantity will be added to the whiskey. So you will get 10 whiskey received and 0 beer.

Copy link

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

Please fix an issue decribed above

Copy link

@Bearnard21 Bearnard21 left a comment

Choose a reason for hiding this comment

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

Functionality LGTM

Copy link

@dmitriypaulov dmitriypaulov left a comment

Choose a reason for hiding this comment

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

Code LGTM, good job!

@dessanhemrayev
Copy link
Author

dessanhemrayev commented May 16, 2024

@dessanhemrayev Thanks for this. Could you point to the depending PR(s) in this one description in order to make review easier ?

@rousseldenis I have updated the description)
check please)

@Bearnard21
Copy link

@rousseldenis Could you please check the PR. Thnx

@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch 4 times, most recently from 48108b4 to 843db6e Compare June 15, 2024 15:56
@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch from de2cbde to 70b702f Compare June 30, 2024 21:49
Copy link

@hoangtrann hoangtrann left a comment

Choose a reason for hiding this comment

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

Looks good in general, well done 💯

stock_picking_auto_create_lot_qty/models/stock_picking.py Outdated Show resolved Hide resolved
stock_picking_auto_create_lot_qty/models/stock_picking.py Outdated Show resolved Hide resolved
stock_picking_auto_create_lot_qty/models/stock_picking.py Outdated Show resolved Hide resolved
stock_picking_auto_create_lot_qty/README.rst Outdated Show resolved Hide resolved
stock_picking_auto_create_lot_qty/README.rst Outdated Show resolved Hide resolved
stock_picking_auto_create_lot_qty/models/stock_picking.py Outdated Show resolved Hide resolved
Comment on lines 105 to 107
"""
Allows to be called either by button or through code
"""

Choose a reason for hiding this comment

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

docstring is incomplete, please provide what this function does

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll fix it

Comment on lines +9 to +12
batch_uom_id = fields.Many2one(
string="Uom",
help="The unit of measure used when creating batches.",
comodel_name="uom.uom",

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

This is one of the conditions of the customer, it is necessary to create a batch

stock_picking_auto_create_lot_qty/readme/CONFIGURE.md Outdated Show resolved Hide resolved
stock_picking_auto_create_lot_qty/readme/USAGE.md Outdated Show resolved Hide resolved
@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch 2 times, most recently from 4461f75 to 3f09ce6 Compare July 4, 2024 19:14
@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). 🤖

Comment on lines 43 to 49
#. Go to *Inventory > Master Data > Products*, select a product. #. Set
'Auto create lot' option, and set 'Create lot every' count of UoMs for
the products you need. #. Go the *Settings > Inventory > Stock Serial
Lot*. #. Press --> Sequences button to create/configure your lot
sequence for a company in 'ir.sequence' module. The default sequence is
'stock.lot.serial'. #. Once a sequence is created/updated select the
stock lot sequence for the current company.

Choose a reason for hiding this comment

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

these still won't appear correctly on the web view, can you fix this?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks,I'll fix it

Comment on lines 56 to 61
#. Go to *Inventory* tab of a product. #. Set a tracking option for this
product. #. Set 'Auto create lot'. #. Go to *Inventory > Incoming* and
create a new transfer. #. Validate picking without lot. #. If 'Only
multiples allowed' has been selected in the product, picking validation
will be blocked if qty received is not a multiple of the value set in
the product.

Choose a reason for hiding this comment

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

these still won't appear correctly on the web view, can you fix this?

Copy link
Author

Choose a reason for hiding this comment

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

I'll fix it

@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch 2 times, most recently from 1aefc28 to b6c469f Compare July 6, 2024 12:15
@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch 2 times, most recently from 5981be7 to 4eacb70 Compare September 8, 2024 17:04
@rousseldenis
Copy link
Contributor

@rousseldenis Please merge PR. Thank you.

@Bearnard21 This PR seems to depends on #1565

@dessanhemrayev
Copy link
Author

@rousseldenis Please merge PR. Thank you.

@Bearnard21 This PR seems to depends on #1565

Hi @rousseldenis )
Thanks
Now this PR does not depend on PR #1565

Copy link

@hoangtrann hoangtrann left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested and it run as expected 👍

@dessanhemrayev
Copy link
Author

@rousseldenis check please)

@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch from 4eacb70 to 7cd2113 Compare October 14, 2024 19:27
@ivs-cetmix
Copy link
Member

Hello @OCA/logistics-maintainers any chances to have this reviewed and merged? )

Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

Migration LG, but we are missing some git commits (translations) and pre-commit commit doesn't include the name of the module updated, and its content seems wrong (probably because the history of 14.0 branch wasn't well retrieved).

https://github.com/OCA/stock-logistics-workflow/commits/14.0/stock_picking_auto_create_lot_qty

Can you check this please?

@ivs-cetmix
Copy link
Member

Migration LG, but we are missing some git commits (translations) and pre-commit commit doesn't include the name of the module updated, and its content seems wrong (probably because the history of 14.0 branch wasn't well retrieved).

https://github.com/OCA/stock-logistics-workflow/commits/14.0/stock_picking_auto_create_lot_qty

Can you check this please?

Sure, thank you for the feedback!

@rvalyi
Copy link
Member

rvalyi commented Dec 19, 2024

@ivs-cetmix it seems you messed it up in your last commit: you merged the 16,0 branch in your migration branch instead of rebasing the migration branch onto 16.0...

Assuming you were on your migration branch, you should have done:

git fetch origin 16.0:16.0
git rebase 16.0

As for now you should delete the merge commit. It can be done with git rebase -I for instance.,,

@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch from 0ebe551 to 5ab04d2 Compare December 19, 2024 20:15
@dessanhemrayev
Copy link
Author

@rvalyi Thanks)
Is it okay now?

@rvalyi
Copy link
Member

rvalyi commented Dec 19, 2024

@rvalyi Thanks)
Is it okay now?

as for the commit it looks OK. I would have expected the weblate translations to come before the migration commits but it's not a big deal either.

rvalyi
rvalyi previously requested changes Dec 19, 2024
Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

almost OK but as per the migration to v15 step you should have replaced SavePointCase by TransactionCase is the tests, see https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-15.0

@rousseldenis
Copy link
Contributor

@dessanhemrayev Could you remove all the commits concerning other modules (translations)? Thanks

@dessanhemrayev dessanhemrayev force-pushed the 16.0-t3463-stock_picking_auto_create_lot_qty-migration branch from 5ab04d2 to 9745b15 Compare December 20, 2024 21:04
@dessanhemrayev
Copy link
Author

almost OK but as per the migration to v15 step you should have replaced SavePointCase by TransactionCase is the tests, see https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-15.0

Hi @rvalyi
In tests I use TransactionCase
maybe I don't understand you?

@dessanhemrayev
Copy link
Author

Migration LG, but we are missing some git commits (translations) and pre-commit commit doesn't include the name of the module updated, and its content seems wrong (probably because the history of 14.0 branch wasn't well retrieved).

https://github.com/OCA/stock-logistics-workflow/commits/14.0/stock_picking_auto_create_lot_qty

Can you check this please?

Hi @sebalix
Thanks for feedback!

could you tell me how I can correctly restore these commits, maybe it would be better for me to do the migration again on the new PR?

@rvalyi
Copy link
Member

rvalyi commented Dec 20, 2024

almost OK but as per the migration to v15 step you should have replaced SavePointCase by TransactionCase is the tests, see https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-15.0

Hi @rvalyi
In tests I use TransactionCase
maybe I don't understand you?

Hum indeed, sorry it seems I looked at a bad place I think.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants