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

[18.0][ADD] calendar_public_holiday #142

Draft
wants to merge 2 commits into
base: 18.0
Choose a base branch
from

Conversation

xaviedoanhduy
Copy link

@xaviedoanhduy xaviedoanhduy commented Dec 26, 2024

context:

note:

  • rename the hr.holidays.public model and related models to calendar.public.holiday to match the Calendar repo.
  • fix words like Holidays Public to Public holidays (because it is misspelled)
  • moving tests and modifications from the old module will only concern the scope of functions in this module.

Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Code and functional review.

IMO the copyright of the corresponding files must be maintained to respect the attribution of all because this module is a “split” of the already existing hr_holidays_public module.

@xaviedoanhduy xaviedoanhduy force-pushed the 18.0-add-calendar_public_holidays branch from 32696bf to aba0190 Compare January 10, 2025 02:37
@xaviedoanhduy
Copy link
Author

Code and functional review.

IMO the copyright of the corresponding files must be maintained to respect the attribution of all because this module is a “split” of the already existing hr_holidays_public module.

thanks for your suggestion, I have updated the copyright.

Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

@pedrobaeza
Copy link
Member

Isn't the migration scripts missing here?

@chienandalu
Copy link
Member

Isn't the migration scripts missing here?

I'd say that the hr_holidays_public stuff relative to time-off is goint to be still needed so the migration work could be done there, isn't it?

@pedrobaeza
Copy link
Member

pedrobaeza commented Jan 10, 2025

I think it should be done here, as the public holidays should be transferred here.

@xaviedoanhduy
Copy link
Author

DRAFT: ADDING MIGRATION SCRIPT DATA

@xaviedoanhduy xaviedoanhduy marked this pull request as draft January 13, 2025 03:52
@xaviedoanhduy xaviedoanhduy force-pushed the 18.0-add-calendar_public_holidays branch from aba0190 to cad993b Compare January 13, 2025 03:56
@xaviedoanhduy xaviedoanhduy force-pushed the 18.0-add-calendar_public_holidays branch from cad993b to bd79fb1 Compare January 13, 2025 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants