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

Ccap 115 primary parent working hours #271

Merged
merged 8 commits into from
May 28, 2024

Conversation

analoo
Copy link
Contributor

@analoo analoo commented May 23, 2024

🔗 Jira ticket

✍️ Description

  • Moved Shawn's Schedule logic into it's own class
  • broke out logic from main method that creates single field submission fields
  • Added test coverage
  • Implemented Job Schedule logic

📷 Design reference

✅ Completion tasks

  • Added relevant tests
  • Meets acceptance criteria

@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 23, 2024 15:22 Inactive
@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 23, 2024 17:33 Inactive
@analoo analoo marked this pull request as ready for review May 23, 2024 18:25
@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 23, 2024 18:25 Inactive
@rapicastillo rapicastillo requested review from coltborg, rapicastillo and enyia21 and removed request for rapicastillo and coltborg May 24, 2024 17:46
Copy link
Contributor

Choose a reason for hiding this comment

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

This class feels more like a utility to me and fits better in utils package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to call it ScheduleUtilities for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I called it SchedulePreparerUtilities so that it is clear where it is being used.

Copy link
Contributor

@rapicastillo rapicastillo left a comment

Choose a reason for hiding this comment

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

Really nice refactoring, I think! cleans up the code a lot and makes sense to move things in a more logical util setup. :)

I posed a suggestion on where to put SchedulePreparer and maybe rename it to ScheduleUtilities just mainly for consistency on utilities, but I'd defer to the rest of the team who has done more work on the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to call it ScheduleUtilities for consistency?

@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 24, 2024 21:25 Inactive
@analoo analoo requested a review from rapicastillo May 24, 2024 21:29
@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 24, 2024 21:53 Inactive
@analoo analoo force-pushed the CCAP-115-primary-parent-working-hours branch from 546ae43 to 5bab344 Compare May 24, 2024 22:22
@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 24, 2024 22:22 Inactive
@analoo analoo requested a review from cram-cfa May 24, 2024 22:29
@enyia21 enyia21 temporarily deployed to il-gcc-ccap-115-primary-8l7ich May 28, 2024 16:05 Inactive
@analoo analoo dismissed rapicastillo’s stale review May 28, 2024 16:05

Addressed the changes

@analoo analoo merged commit d78aad5 into main May 28, 2024
5 checks passed
@analoo analoo deleted the CCAP-115-primary-parent-working-hours branch May 28, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants