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

Add endpoint and function to set the current operating cycle #65

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

stuartcampbell
Copy link
Collaborator

@stuartcampbell stuartcampbell commented May 1, 2024

I have restricted access to the endpoint to admin API keys

@stuartcampbell stuartcampbell added the enhancement New feature or request label May 1, 2024
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.

I think adding a transaction / context manager would be a small change to add resilience to operations like set_current_operating_cycle(). The rest looks great.

src/nsls2api/services/proposal_service.py Outdated Show resolved Hide resolved
src/nsls2api/services/facility_service.py Outdated Show resolved Hide resolved
src/nsls2api/services/facility_service.py Outdated Show resolved Hide resolved
Comment on lines +122 to +131
if previous_cycle is not None:
await previous_cycle.set({Cycle.is_current_operating_cycle: False})

await new_current_cycle.set({Cycle.is_current_operating_cycle: True})

# Let's now check all is well with the world
expected_current_cycle = await current_operating_cycle(facility)
if str(expected_current_cycle) != cycle:
logger.error(f"Failed to set the current operating cycle for {facility} to be {cycle}.")
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably packaged into a transaction (or at least use some context manager) to rollback on failure and avoid an inconsistent state.

The test at the end could be removed at that point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants