-
Notifications
You must be signed in to change notification settings - Fork 7
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 LBMS commissioning proposal support #145
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at this and proposal_service.is_commissioning()
, it seems like the pass_type_id
field is actually crucial for handling commissioning proposals - we should make this a mandatory field (is currently Optional).
looks comprehensive in getting the functions we need, otherwise!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass_service also needs to be updated to use the appropriate commissioning proposal type
diff --git a/src/nsls2api/services/pass_service.py b/src/nsls2api/services/pass_service.py
index 747659a..fccbf5c 100644
--- a/src/nsls2api/services/pass_service.py
+++ b/src/nsls2api/services/pass_service.py
@@ -149,9 +149,8 @@ async def get_commissioning_proposals_by_year(
error_message: str = f"Facility {facility_name} does not have a PASS ID."
logger.error(error_message)
raise PassException(error_message)
-
- # The PASS ID for commissioning proposals is 300005
- url = f"{base_url}/Proposal/GetProposalsByType/{api_key}/{pass_facility}/{year}/300005/NULL"
+ pass_commissioning_type = await get_commissioning_proposal_type(facility_name)
+ url = f"{base_url}/Proposal/GetProposalsByType/{api_key}/{pass_facility}/{year}/{pass_commissioning_type.pass_id}/NULL"
try:
pass_commissioning_proposals = await _call_pass_webservice(url)
@JunAishima Can you open a PR against this PR branch with that change? |
Yes, that will work well - I am looking for any further issues I can handle in the same PR stuartcampbell#2 (Note, Stu's github site) |
I've tested this using a local instance of nsls2api against production PASS (due to issues with passdev) and, after including Jun's changes, I think this is working.
With this we can sidestep for now the question of recording facilities in the proposal data model. Though as we discussed, there is more thinking needed on that point (for example, this won't fix facility filtering for the newer proposals endpoint) |
I will remove this request for the current PR and have made an issue - #149 Approving PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve - also requires stuartcampbell#2 to fully handle LBMS proposals correctly.
This PR adds in support to deal with LBMS facilities commissioning proposals.
There are a number of places that this code can be improved by removing hard coded values for the PASS IDs for the respective commissioning proposal types (I've created an issue #146 )
Testing on my local dev instance...
http://127.0.0.1:8000/v1/proposals/commissioning?facility=lbms
http://127.0.0.1:8000/v1/proposals/commissioning?facility=nsls2
http://127.0.0.1:8000/v1/proposals/commissioning?facility=cfn
And with no facility parameter... http://127.0.0.1:8000/v1/proposals/commissioning
And specifying a beamline http://127.0.0.1:8000/v1/proposals/commissioning?beamline=PDF
Which still gives the same answer if we select the wrong facility for the beam line
http://127.0.0.1:8000/v1/proposals/commissioning?beamline=PDF&facility=lbms
I think that this will also fix #140