Skip to content

Conversation

@HarikaBishai
Copy link
Collaborator

@HarikaBishai HarikaBishai commented Aug 13, 2025

Added:

  • Logic to fetch the cycle for today's date in facility_cycle_by_date
  • Logic to set the current cycle after sync in worker_synchronize_cycles_from_pass.

@HarikaBishai HarikaBishai marked this pull request as draft August 13, 2025 16:01
@stuartcampbell
Copy link
Collaborator

stuartcampbell commented Aug 13, 2025

Would it make sense to take the logic within set_current_cycle_by_today() and make it a method within facility_service so that it can also just be called at the end of the facility cycle sync method (which I think is called sync_service.worker_synchronize_cycles_from_pass())- as it seems sensible to set the current cycle when we synchronize from PASS ?

@HarikaBishai
Copy link
Collaborator Author

Would it make sense to take the logic within set_current_cycle_by_today() and make it a method within facility_service so that it can also just be called at the end of the facility cycle sync method (which I think is called sync_service.worker_synchronize_cycles_from_pass())- as it seems sensible to set the current cycle when we synchronize from PASS ?

Yeah that makes sense, since this api will be not called directly, it is better to add it in existing method. I'll update and let you know.

@HarikaBishai HarikaBishai changed the title Add admin API to set current cycle by today's date and tests for cycle-by-date and admin endpoints Added cycle automation changes to cycles sync process Aug 13, 2025
@HarikaBishai HarikaBishai changed the title Added cycle automation changes to cycles sync process Added current cycle automation changes to cycles sync process Aug 13, 2025
@HarikaBishai HarikaBishai marked this pull request as ready for review August 13, 2025 18:30
@HarikaBishai
Copy link
Collaborator Author

HarikaBishai commented Aug 13, 2025

Would it make sense to take the logic within set_current_cycle_by_today() and make it a method within facility_service so that it can also just be called at the end of the facility cycle sync method (which I think is called sync_service.worker_synchronize_cycles_from_pass())- as it seems sensible to set the current cycle when we synchronize from PASS ?

Yeah that makes sense, since this api will be not called directly, it is better to add it in existing method. I'll update and let you know.

I have added the changes to worker_synchronize_cycles_from_pass and removed the apis.

@stuartcampbell stuartcampbell requested a review from Copilot August 13, 2025 21:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the cycle synchronization process by automatically setting the current operating cycle based on today's date after syncing cycles from PASS. The changes ensure that the facility's current cycle is updated to match the cycle that contains the current date.

  • Added functionality to find cycles by date range
  • Enhanced cycle synchronization to automatically set current operating cycle
  • Improved documentation and logging for the sync process

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/nsls2api/services/sync_service.py Enhanced worker_synchronize_cycles_from_pass to set current cycle after sync and improved documentation
src/nsls2api/services/facility_service.py Added facility_cycle_by_date function to find cycles containing a specific date

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Copy link
Collaborator

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I suggested some minor copyedits in the comments.

HarikaBishai and others added 2 commits August 18, 2025 13:46
Co-authored-by: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com>
Co-authored-by: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com>
@HarikaBishai
Copy link
Collaborator Author

Looks reasonable to me. I suggested some minor copyedits in the comments.

I have applied the changes.

@padraic-shafer padraic-shafer merged commit 4351834 into NSLS2:main Aug 19, 2025
6 checks passed
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.

3 participants