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

[16.0][ADD] stock_release_channel_partner_public_holidays #751

Merged
merged 1 commit into from
Oct 6, 2023
Merged

[16.0][ADD] stock_release_channel_partner_public_holidays #751

merged 1 commit into from
Oct 6, 2023

Conversation

nguyenminhchien
Copy link
Contributor

@nguyenminhchien nguyenminhchien commented Sep 18, 2023 •
edited by simahawk
Loading

@nguyenminhchien nguyenminhchien force-pushed the 16.0-stock_release_channel_partner_public_holidays branch 4 times, most recently from afc5a57 to 57f536b Compare September 19, 2023 08:46
@nguyenminhchien nguyenminhchien marked this pull request as ready for review September 19, 2023 08:51
cyrilmanuel
cyrilmanuel approved these changes Sep 21, 2023
View reviewed changes
Copy link

@cyrilmanuel cyrilmanuel left a comment

Choose a reason for hiding this comment

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

not an expert but lgtm

@jbaudoux jbaudoux self-requested a review October 5, 2023 07:35
jbaudoux
jbaudoux requested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

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

Code can be simplified

@nguyenminhchien nguyenminhchien force-pushed the 16.0-stock_release_channel_partner_public_holidays branch 4 times, most recently from 2260259 to be84acc Compare October 6, 2023 05:33
jbaudoux
jbaudoux approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

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

Thanks

@simahawk simahawk force-pushed the 16.0-stock_release_channel_partner_public_holidays branch from be84acc to 4168aa9 Compare October 6, 2023 13:44
simahawk
simahawk approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Overall LG, some minor glitches found but can be fixed in another round.
Rebased and dropped test req.

stock_release_channel_partner_public_holidays/models/stock_release_channel.py
Returns True if shipment_date is a public holiday
:return: bool
"""
self.ensure_one()
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no need to call ensure_one for a private method that is used only in this loop

stock_release_channel_partner_public_holidays/models/stock_release_channel.py
("date", "=", shipment_date),
]
hhplo = self.env["hr.holidays.public.line"]
holidays_line = hhplo.search(domain, limit=1, order="id")
Copy link
Contributor

Choose a reason for hiding this comment

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

should use search_count instead

stock_release_channel_partner_public_holidays/readme/CONTRIBUTORS.rst
@@ -0,0 +1 @@
* Nguyen Minh Chien <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

JE missing

@simahawk
Copy link
Contributor

simahawk commented Oct 6, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-751-by-simahawk-bump-nobump, awaiting test results.

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

@OCA-git-bot OCA-git-bot merged commit db87359 into OCA:16.0 Oct 6, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8c050b2. 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
Reviewers

@cyrilmanuel cyrilmanuel cyrilmanuel approved these changes

@jbaudoux jbaudoux jbaudoux approved these changes

@simahawk simahawk simahawk 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.
5 participants
@nguyenminhchien @simahawk @OCA-git-bot @jbaudoux @cyrilmanuel