-
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
Feature: Grant Notes feed #3428 #3532
Conversation
- Implemented `getOrganizationNotesForGrantByUser()` function in `grantsCollaboration/notes.js` - Renamed `getOrganizationNotesForGrant()` to `getCurrentNoteRevisions()` and updated signature - Made conditional `WHERE` clause for `grantId` and `userId` in `getCurrentNoteRevisions()` - Created API route `GET /:grantId/notes/user/:userId` in `grants.js` for fetching user-specific grant notes - Added validation for `afterRevision` and `limit` query parameters in the new API - Ensured results are paginated and ordered by `created_at` in descending order - Added necessary imports/exports in `grantsCollaboration/index.js`
…ific-user-FinderAPI
…t-notes-specific-user-FinderAPI' into feat/grant-notes
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: @greg-adams, Action: |
1827f15
to
5518c9f
Compare
@TylerHendrickson Changes from #3471 are merged and PR can be reviewed |
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.
@greg-adams Overall this is looking great. I have a few suggestions that I'd like your take on ahead of approval.
'r.text', | ||
]) | ||
const subquery = knex | ||
.select(knex.raw(`r.*, count(*) OVER() AS total_revisions`)) |
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.
This is a bit of a nit, but given that we're only interested in knowing whether there is more than 1 revision (rather than the total), I'm wondering if we should limit the potential impact of the window function by moving it to a new layer between the outermost query and the subquery, for example:
-- outer query
LEFT JOIN LATERAL (
SELECT
recent_revs.*,
count(recent_revs.*) OVER() > 1 as is_revised
FROM (
SELECT
r.id,
r.grant_note_id,
r.created_at,
r.text
FROM grant_notes_revisions r
WHERE r.grant_note_id = grant_notes.id
ORDER BY r.created_at DESC
LIMIT 2
) AS recent_revs
ORDER BY recent_revs.created_at DESC
LIMIT 1
) AS rev
The basic idea is that the window function would be prevented from counting more than 2 rows, rather than needing to scan an unbounded number of rows.
PS- I tried to see if just adding > 1
to the window function as it's currently implemented would do this automatically, but it seems that the Postgres query planner still performs the full count beyond just satisfying the > 1
determination.
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.
no worries, I've updated to include this limit
@@ -86,21 +84,29 @@ async function getCurrentNoteRevisions( | |||
} | |||
|
|||
if (afterRevision) { | |||
query = query.andWhere('rev.id', '>', afterRevision); | |||
query = query.andWhere('rev.id', '<', afterRevision); |
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.
Good catch.
Given this, what do you think about renaming the afterRevision
variable name, given that it's currently a bit misleading? A few thoughts on what to do instead:
- Rename to
beforeRevision
. Other than leaving it as-is, this seems the most straightforward. - Rename to
paginateFrom
(similar to the API) orcursor
(more descriptive). Both of these are more generic (and maybe appropriate for standardizing other similar functions in thegrantsCollaboration
package).
Personally, I prefer the second option because they don't imply a direction, which would be better if we eventually decided to expand functionality by allowing callers to specify a direction
.
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.
yes this makes sense - I've updated both to use cursor
naming, and we can add direction
where needed
</b-form-group> | ||
</div> | ||
|
||
<!-- Users Note --> |
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.
<!-- Users Note --> | |
<!-- Current User's Note --> |
async fetchNextNotes() { | ||
const query = { | ||
grantId: this.currentGrant.grant_id, | ||
limit: 4, |
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.
Per a conversation I had with @ClaireValdivia recently about improving test-ability, what do you think of adding this function to packages/client/src/helpers/featureFlags/index.js
, and calling that function on this line?
export function grantNotesLimit() {
return parseInt(getFeatureFlags().grantNotesLimit, 10) || 4;
}
This would enable users to run something like window.APP_CONFIG.overrideFeatureFlag(grantNotesLimit, 1)
and change the pagination size on-the-fly.
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.
very useful idea - added!
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.
@greg-adams Looks great – approved!
Ticket #3428
Description
next
indicating additional set)Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist