Skip to content

[17.0] [MIG] product lot sequence#1708

Merged
OCA-git-bot merged 45 commits intoOCA:17.0from
apikcloud:17.0-mig-product_lot_sequence
Feb 11, 2025
Merged

[17.0] [MIG] product lot sequence#1708
OCA-git-bot merged 45 commits intoOCA:17.0from
apikcloud:17.0-mig-product_lot_sequence

Conversation

@fredericgrall
Copy link
Contributor

No description provided.

@fredericgrall fredericgrall mentioned this pull request Aug 12, 2024
69 tasks
@@ -0,0 +1,28 @@
# Copyright 2023 Camptocamp SA
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl)
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the logger stuff added in a migration commit?
Is the logger needed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bosd, I remove logging import

@bosd
Copy link
Contributor

bosd commented Sep 30, 2024

Thanks, can you please squash that Into the migration commit

@fredericgrall fredericgrall force-pushed the 17.0-mig-product_lot_sequence branch from 1b4b157 to adc6261 Compare September 30, 2024 14:59
@fredericgrall
Copy link
Contributor Author

@bosd squash is done

@bosd
Copy link
Contributor

bosd commented Sep 30, 2024

Thanks, but pre-commit is no longer happy

@fredericgrall fredericgrall force-pushed the 17.0-mig-product_lot_sequence branch from adc6261 to 738eef7 Compare October 8, 2024 09:29
@fredericgrall
Copy link
Contributor Author

@bosd, sorry for the late reply, the pre-commit is good now

Copy link
Contributor

@bosd bosd 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

@Tisho99 Tisho99 left a comment

Choose a reason for hiding this comment

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

Changes requested

from odoo.tests import Form
from odoo.tests.common import TransactionCase

_logger = logging.getLogger(__name__)
Copy link

Choose a reason for hiding this comment

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

I think this is not needed

Comment on lines 8 to 11
<xpath expr="//block[@id='production_lot_info']" position="inside">
<div class="col-12 col-lg-6 o_setting_box">
<div class="o_setting_right_pane">
<label for="lot_sequence_padding" />
Copy link

Choose a reason for hiding this comment

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

@Tisho99
Copy link

Tisho99 commented Feb 3, 2025

Hi @fredericgrall

Thanks for the work done

You are able and willing to continue the PR?.

Could you rebase?

If not i will do a supersede

@fredericgrall fredericgrall force-pushed the 17.0-mig-product_lot_sequence branch 2 times, most recently from 40f6d58 to baec8e5 Compare February 4, 2025 04:38
@fredericgrall
Copy link
Contributor Author

Hi @Tisho99, Thanks for your comments, I've rebased

Copy link

@Tisho99 Tisho99 left a comment

Choose a reason for hiding this comment

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

LGTM!

Code and functional review

However, the PR title should be: [17.0] [MIG] product_lot_sequence

Can you change it?

@fredericgrall fredericgrall force-pushed the 17.0-mig-product_lot_sequence branch from baec8e5 to 6066f1f Compare February 4, 2025 08:46

def setUp(self):
super(TestProductLotSequence, self).setUp()
super().setUp()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change to classmethod ?

@rousseldenis
Copy link
Contributor

/ocabot migration product_lot_sequence

@Tisho99
Copy link

Tisho99 commented Feb 7, 2025

Hi @fredericgrall

Could you please make the last 2 small requested changes?

The migration is almost complete and it would be a shame if the PR is forgotten again

Thank you!

@fredericgrall
Copy link
Contributor Author

Hi @Tisho99
I think I've made the two changes requested

@HaraldPanten
Copy link

Hi @fredericgrall I'm not sure if you attended Denis's comments: #1708 (comment)

You can improve the title of the PR, as @Tisho99 commented here: #1708 (review)

THX!

@fredericgrall fredericgrall changed the title 17.0 mig product lot sequence [17.0] [MIG] product lot sequence Feb 7, 2025
@fredericgrall
Copy link
Contributor Author

@Tisho99 @HaraldPanten , sorry, I've rename PR

Comment on lines +7 to +8

def setUp(self):
Copy link

Choose a reason for hiding this comment

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

Suggested change
def setUp(self):
@classmethod
def setUpClass(self):

Copy link

Choose a reason for hiding this comment

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

This is what rousseldenis requested, can you do it? Its for speeding up tests

@fredericgrall fredericgrall force-pushed the 17.0-mig-product_lot_sequence branch from 6066f1f to cc20859 Compare February 11, 2025 05:16
@fredericgrall
Copy link
Contributor Author

@Tisho99, thank you for your help, I've made the correction

@fredericgrall fredericgrall force-pushed the 17.0-mig-product_lot_sequence branch from cc20859 to d2f1ba6 Compare February 11, 2025 05:22
Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

LGTM. Merging as Denis's comments have been corrected.

@HaraldPanten
Copy link

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1708-by-HaraldPanten-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9894e82 into OCA:17.0 Feb 11, 2025
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.