-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(grants): Forecasted UI - - MERGE AFTER PR # 3456 #3346
base: main
Are you sure you want to change the base?
Conversation
@TylerHendrickson would you mind taking a look at this draft PR to make sure I'm on the right track as far as implementation goes for this ticket |
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.
@TylerHendrickson would you mind taking a look at this draft PR to make sure I'm on the right track as far as implementation goes for this ticket
@masimons Left a few notes for clarification / consideration, but I think overall this is on the right track!
} if (this.currentGrant.opportunity_status === 'forecasted') { // what if we check for posted status as well? | ||
if (!this.currentGrant.close_date) { | ||
return this.currentGrant.close_date_explanation ? this.currentGrant.close_date_explanation : 'Not yet issued'; | ||
} |
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.
Seems like this check would also be useful for posted status.
I think this is already on your radar, but just calling out for posterity that this (undesired) fallback behavior would need to be addressed in order to have potentially-null close_date
values.
// Note: in the knex query, we are deriving the status based on | ||
// archive_date and close_date. Probably we just want to pass | ||
// the opportunity_status to the frontend, and not re-derive it here? |
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.
Unlike the transition from forecasted to non-forecasted, there's (currently) no explicit grants-ingest event that we expect to receive that would allow us to store a new materialized value for the grant being closed or archived. In other words, to determine whether a grant is forecasted or fully published, we have these options:
- Check
is_forecasted
/ some value that signifies whether the upstream record was sourced only from forecasted data - Check whether
post_date
is in the future
However, the post_date
may well be the last time we ever see an event from grants-ingest for a particular grant, so the status of being closed
or archived
need to be derived from a check like
if (close_date <= today) {
if (archive_date > today) {
status = 'closed';
} else {
status = 'archived';
}
}
(Hopefully that's useful and I'm not misunderstanding the question!)
99a36c9
to
f9aea32
Compare
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation Output
Plan Summary
Pusher: @lsr-explore, Action: |
f9aea32
to
f2914c8
Compare
29b6b60
to
830ccf5
Compare
830ccf5
to
1e0dde0
Compare
@@ -219,7 +219,7 @@ import GrantActivity from '@/components/GrantActivity.vue'; | |||
|
|||
const HEADER = '__HEADER__'; | |||
const FAR_FUTURE_CLOSE_DATE = '2100-01-01'; | |||
const NOT_AVAILABLE_TEXT = 'Not available'; | |||
const NOT_AVAILABLE_TEXT = 'Not Available'; |
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.
Resolved - Fix is in this PRFigma shows est in italics for Open/Close date fix is in this PR |
17e2fdf
to
dd41ced
Compare
]) | ||
.select(knex.raw(` | ||
CASE | ||
WHEN grants.archive_date <= now() THEN 'archived' | ||
WHEN grants.close_date <= now() THEN 'closed' | ||
WHEN grants.open_date > now() OR grants.opportunity_status = 'forecasted' THEN 'forecasted' |
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.
Note to reviewer: Additional check for open_date in case the status is not correctly populated in grants.gov
}, | ||
displayEstimatedOpenDateText() { | ||
return this.currentGrant.open_date && this.currentGrant.opportunity_status === 'forecasted'; | ||
}, |
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.
Note to reviewer: We do not want to display prefix "est." if the dates are null and the status is forecasted.
valid date - "est. 1/2/2056"
date = null - "not yet issued"
We need this as a separate flag because the requirement is to display "est." in italics.
dd41ced
to
f565549
Compare
Blocked from merging
Blocked by - #3456 - Hide forecasted grants while UI is being developed to support forecasted grants
Ticket #2201
Note
I tried adding tests, but the setup is missing for being able to do a full render - may need to wait until we upgrade ViTest.
See slack thread for more details
Description
UI changes that display forecasted grant data. The bulk of these changes touch the grants table, and the grants detail page.
To run this...
Screenshots / Demo Video
Grants table with forecasted and posted grants
csv output
grant detail - both dates valid
grant detail - with close_date_explanation
grant detail - both dates = null
sorting on close date
Manually changed data - see
Filtering
Posted only
Forecasted only
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist