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.

Sign up for GitHub

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

Jump to bottom

feat(your-work): Jira Integration pagination #8916

Merged
merged 5 commits into from
Oct 12, 2023
Merged

feat(your-work): Jira Integration pagination #8916

merged 5 commits into from
Oct 12, 2023

Conversation

jmtaber129
Copy link
Contributor

@jmtaber129 jmtaber129 commented Oct 3, 2023 •
edited
Loading

Description

Fixes #8917

Pagination for Your Work - Jira.

Note that because this modifies the Jira GQL API (both pagination args and behavior) there is a risk of regression in the Jira integration for Sprint Poker, so that should be smoke tested more rigorously than usual.

Needs rebase once #8907 lands

Demo

Multiple pages (2 per page):
https://www.loom.com/share/83e737abb0bc4c30a1b61f9d20bd29f3?sid=469f4e61-0ecb-44a4-9c40-f7e1dc92cdc2

Paginate on scroll to bottom (7 per page):
https://www.loom.com/share/7dcf95f1fe6249faad5d9fac9f6609ec?sid=112a19e7-eecd-4a06-96a5-1468a95fd8a2

Testing scenarios

  • Connect a team to Jira, and open a standup meeting on that team
  • Assign several issues to yourself in Jira
  • Set the page size to a number less than the number of assigned issues
  • Confirm that when the end of the list is seen by the user, the next page of issues is loaded.
  • Confirm that when there are no more assigned issues, the end of the list does not show a loading animation.
  • Smoke test sprint poker for Jira.

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label One Review Required if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

@github-actions github-actions bot added the size/m label Oct 3, 2023
@jmtaber129 jmtaber129 force-pushed the jmtaber129/your-work-jira-pagination branch 2 times, most recently from c23153b to ceff10b Compare October 3, 2023 20:24
jmtaber129
jmtaber129 commented Oct 5, 2023
View reviewed changes
packages/server/graphql/private/typeDefs/_legacy.graphql
@@ -935,7 +935,7 @@ type JiraIssueEdge {
The item at the end of the edge
"""
node: JiraIssue!
cursor: DateTime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This connection wasn't truly implemented previously, since our GQL API only accepted a first arg.

Using a DateTime cursor here makes things confusing when paginating through items in descending date order, since we'd be querying for everything before the after date. This also goes against relay best-practices that specify that cursors should be opaque to clients.

Using a normal String cursor also allows us to forward the first + before GQL connection args directly to the Atlassian API, eliminating complexity around converting our GQL pagination to the Atlassian API's pagination.

This also increases consistency with how the Jira Server GQL API is implemented

@jmtaber129 jmtaber129 marked this pull request as ready for review October 5, 2023 16:31
BartoszJarocki
BartoszJarocki approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@BartoszJarocki BartoszJarocki left a comment

Choose a reason for hiding this comment

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

just one little thing, nicely done otherwise!

packages/server/graphql/public/types/AtlassianIntegration.ts
// Request one extra item to see if there are more results
const maxResults = first + 1
// Relay requires the cursor to be a string
const afterInt = parseInt(after ?? '-1', 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 technically after is a string so could be anything, and then parseInt will return NaN. would be good to check agains that

@jmtaber129 jmtaber129 force-pushed the jmtaber129/your-work-jira branch from 7be1a4f to 29193eb Compare October 10, 2023 22:42
@jmtaber129 jmtaber129 force-pushed the jmtaber129/your-work-jira-pagination branch from d6d9731 to 8095f45 Compare October 10, 2023 22:42
Dschoordsch
Dschoordsch requested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Only did short smoke test

packages/server/graphql/public/types/AtlassianIntegration.ts Outdated
const maxResults = first + 1
// Relay requires the cursor to be a string
const afterInt = parseInt(after ?? '-1', 10)
const startAt = Number.isNaN(afterInt) ? -1 : afterInt + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 if after === null then afterInt === -1 and startAt === 0.
If after === 'invalid' then afterInt = NaN and startAt === -1.
This seems a bit odd.

-1 I see we're using this value for cursor calculation, this will be wrong then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea this is a mistake - updated the truthy side of the ternary from -1 to 0

Base automatically changed from jmtaber129/your-work-jira to master October 11, 2023 16:22
@jmtaber129 jmtaber129 force-pushed the jmtaber129/your-work-jira-pagination branch from 8095f45 to e1ad8d9 Compare October 11, 2023 16:38
Dschoordsch
Dschoordsch approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Are you going to fix pagination in poker scope as well? Otherwise can you please make sure we have an issue for it?

@jmtaber129 jmtaber129 mentioned this pull request Oct 12, 2023
Closed
2 tasks
@jmtaber129
Copy link
Contributor Author

Created #8962 to track pagination for Jira in Poker

@jmtaber129 jmtaber129 merged commit 33ed3c1 into master Oct 12, 2023
@jmtaber129 jmtaber129 deleted the jmtaber129/your-work-jira-pagination branch October 12, 2023 14:15
@github-actions github-actions bot mentioned this pull request Oct 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dschoordsch Dschoordsch Dschoordsch approved these changes

@BartoszJarocki BartoszJarocki BartoszJarocki approved these changes

Assignees
No one assigned
Labels
size/m
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Your Work: Jira - pagination
3 participants
@jmtaber129 @BartoszJarocki @Dschoordsch