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.

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

switched away from deprecated GetCreateIssueMeta to get the list of p… #849

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

levb
Copy link
Contributor

@levb levb commented Mar 16, 2022

There was a customer report that not all projects were showing in the project list ("edit subscription")

This PR switches away from using the GetCreateIssueMeta which is deprecated, in favor of cloud|server-specific APIs to get the list of all projects.

cc @hanzei

@levb levb added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 16, 2022
@levb levb requested a review from mickmister as a code owner March 16, 2022 03:54
@codecov-commenter
Copy link

Codecov Report

Merging #849 (1d9b1e9) into master (f986c15) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
- Coverage   31.46%   31.25%   -0.21%     
==========================================
  Files          49       49              
  Lines        5982     6015      +33     
==========================================
- Hits         1882     1880       -2     
- Misses       3911     3946      +35     
  Partials      189      189              
Impacted Files Coverage Δ
server/client.go 9.25% <0.00%> (+0.31%) ⬆️
server/client_cloud.go 0.00% <0.00%> (ø)
server/client_server.go 0.00% <0.00%> (ø)
server/issue.go 31.78% <0.00%> (+0.05%) ⬆️
server/plugin.go 11.02% <0.00%> (-0.05%) ⬇️
server/subscribe.go 67.61% <0.00%> (-0.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f986c15...1d9b1e9. Read the comment docs.

IsLast bool `json:"isLast"`
}

remaining := 50
Copy link

@catalintomai catalintomai Mar 16, 2022

Choose a reason for hiding this comment

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

@levb - just to confirm, this value looks to be useful only when limit is 0. Since we use only "-1" as a value in the negative spectrum, maybe we could've used limit = 0 to indicate fetchAll and only pass >= 0 values for limit instead. 0/5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the default. The limit is not currently used anywhere, we always use -1 to get all projects.

Copy link
Contributor Author

@levb levb Mar 16, 2022

Choose a reason for hiding this comment

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

Also, when we fetch all (-1), we do it 50 at a time.

for _, issue := range prj.IssueTypes {
if issue.Subtasks {
if issue.Subtask {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting change here

@levb levb removed the 2: Dev Review Requires review by a core committer label Mar 17, 2022
@DHaussermann
Copy link

DHaussermann commented Mar 22, 2022

@levb @dipak-demansol and I are unable to reproduce this on Cloud or Server with the production Jira release. In both cases 50+ projects can be loaded into the Jira create issue modal. Perhaps there is more detail about what functionality is affected in a Jira task somewhere.

Note we could just regression test but, I'd feel more confident if we can see the issue manifesting.

@levb
Copy link
Contributor Author

levb commented Mar 22, 2022

1/5 regression testing is all that is needed. @DHaussermann

@dipak-demansol dipak-demansol self-requested a review April 1, 2022 11:06
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed
We were unable to reproduce the original customer report however using Lev's new mechanism for retrieving the projects was regression tested.

  • Confirmed with datasets between 50 - 100 projects
  • All projects are shown in the issue create modal
  • All projects are showing in subscription edit modal
  • No related error found in browser console when using the functionality
  • Tested on browser and Desktop

LGTM!

@dipak-demansol dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels May 11, 2022
Copy link

@dipak-demansol dipak-demansol left a comment

Choose a reason for hiding this comment

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

@levb levb merged commit b362293 into master Jul 13, 2022
@levb levb deleted the lev-get-projects-paginated branch July 13, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants