-
Notifications
You must be signed in to change notification settings - Fork 3
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
2962: CAN Summary Card - update funding summary endpoint #3075
base: main
Are you sure you want to change the base?
Conversation
eaca2c4
to
b7f1898
Compare
@@ -172,7 +172,7 @@ def register_api(api_bp: Blueprint) -> None: | |||
) | |||
|
|||
api_bp.add_url_rule( | |||
"/can-funding-summary/<int:id>", |
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.
The endpoints for *ITEM_API_VIEW*
are designed to take an resource id, e.g. <int:id>
while the *LIST_API_VIEW
does not. It would be more consistent if /can-funding-summary/
used CAN_FUNDING_RECEIVED_LIST_API_VIEW_FUNC
.
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.
Great catch! I changed the view name to CAN_FUNDING_SUMMARY_LIST_API_VIEW_FUNC
.
|
||
# Check if required 'can_ids' parameter is provided | ||
if not can_ids: | ||
return make_response_with_headers({"error": "'can_ids' parameter is required"}, 400) |
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.
The above request.args
handling and can_ids
validation would be better in a Marshmallow schema but it would be acceptable to take that as a tech debt item if you don't have time to implement it.
@@ -87,15 +108,109 @@ def get_can_funding_summary(can: CAN, fiscal_year: Optional[int] = None) -> CanF | |||
available_funding = total_funding - sum([planned_funding, obligated_funding, in_execution_funding]) or 0 | |||
|
|||
return { | |||
"can": can.to_dict(), |
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.
Would be better as a Marshmallow schema for the response but OK as tech debt.
* fix: ops api url for funding sums * refactor: be logic * feat: apply fy to filter
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.
🐝 swarming on this past few days and it's working great
inExecutionFunding={fundingSummaryData?.in_execution_funding} | ||
/>} | ||
/> | ||
<DebugCode |
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.
👀 let's remove this before merging in
What changed
Endpoint
/api/v1/can-funding-summary
changes:new_funding
,in_draft_funding
Front-end (FE) changes:
Issues
Update CAN Funding summary endpoint #2962
Add DRAFT budget lines to /can-funding-summary response #3070
How to test
Ensure all unit and E2E tests are passing.
Screenshots
N/A
Definition of Done Checklist
- [ ] Form validations updatedLinks
New Funding