Update iteration cadence mutation [RUN ALL RSPEC] [RUN AS-IF-FOSS]
What does this MR do?
Adds GraphQL support for updating iteration cadences. #322742 (closed)
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
- Resolved by Heinrich Lee Yu
Hey @acroitor. Here's a WIP on what we discussed today, cherry-picked your commit. I still have to work on the update service but if you could take a look to see if you already have some comments I'd really appreciate it. Specially around global ids and how to find the object, not sure if that's the right/current way to do it. Thanks!
Hi @mcelicalderonG,
Please add labels to your merge request, this helps triage community merge requests.
Thanks for your help!
❤
You are welcome to help improve this comment.
added auto updated label
- Resolved by Heinrich Lee Yu
- Resolved by Heinrich Lee Yu
added GraphQL backend groupproject management sectiondev labels
removed auto updated label
added 470 commits
-
222f0e5e...95099ebb - 468 commits from branch
master
- 187c51b1 - Add create iteration cadence service and GraphQL mutation
- 219873ba - WIP [ci skip]
-
222f0e5e...95099ebb - 468 commits from branch
added 2 commits
added databasereview pending label
added database documentation labels
2 Messages 📖 This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. 📖 CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, run the following:
bin/changelog -m 55439 "Update iteration cadence mutation [RUN ALL RSPEC] [RUN AS-IF-FOSS]"
If you want to create a changelog entry for GitLab EE, run the following instead:
bin/changelog --ee -m 55439 "Update iteration cadence mutation [RUN ALL RSPEC] [RUN AS-IF-FOSS]"
If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Documentation review
The following files require a review from a technical writer:
doc/api/graphql/reference/index.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Valery Sizov ( @vsizov
) (UTC+1)Charlie Ablett ( @cablett
) (UTC+13)If needed, you can retry the
danger-review
job that generated this comment.Generated by
🚫 DangerEdited by 🤖 GitLab Bot 🤖- Resolved by Alexandru Croitor
@acroitor please take another look when you get a chance, specifically at this commit as the others are cherry-picked or copied from your branches and I will remove later.
Some notes:
- Still missing update service specs as I just copied from iterations so far, will complete tomorrow.
- Must check that the feature is enabled in the mutation before actually getting to the service as the cadence finder will raise an error if I try to use it with the feature disable (which I think is good). I need the finder before the update service to authorize the cadence.
Thanks!
added 284 commits
-
c167dbc2...9564caac - 281 commits from branch
master
- 6eb67d52 - Add create iteration cadence service and GraphQL mutation
- 5a2ab9a7 - Temp add finder
- f182b774 - Iteration cadences update mutation
Toggle commit list-
c167dbc2...9564caac - 281 commits from branch
- Resolved by Alexandru Croitor
Hey @acroitor I think this one is ready for review, but I guess we have to wait for your MRs to get merged first. I was also wondering about something now that I understand how authorization works a bit better. I know we discussed this a bit, but still wondering if this makes sense:
- Right now I use the CadenceFinder to make sure I lookup for cadences only in groups I have access to (not passing the ancestors args so right now it would only find cadences for the actually provided group path).
- I'm not sure if we should do a search like this if the only thing we are trying to do is update a cadance by ID and of course make sure I'm authorized to do that.
- Doing an
authorized_find!(id: id)
will internally call
Ability.allowed?(current_user, :admin_iteration_cadence, cadence, scope: :user)
The
Iterations::CadencePolicy
delegates authorization to the Cadence's group, so what this will do is just make sure that the current user has the ability to:admin_iteration_cadence
on the group which means:- Iterations feature is available for the group
- User has at least developer access on the cadence's group or one of it's ancestors.
Please let me know if any of this makes sense. Thanks!
requested review from @acroitor
Setting label(s) devopsplan based on groupproject management.
added devopsplan label
added 743 commits
-
cfbdc913...a95edad7 - 742 commits from branch
master
- 8bf378eb - Add update iteration cadence GraphQL mutation
-
cfbdc913...a95edad7 - 742 commits from branch
changed milestone to %13.10
added 32 commits
-
8bf378eb...8d51d2c8 - 31 commits from branch
master
- 3f5b0659 - Add update iteration cadence GraphQL mutation
-
8bf378eb...8d51d2c8 - 31 commits from branch
- Resolved by Mario Celi
requested review from @krasio
added workflowin review label
removed workflowin review label
mentioned in issue #322742 (closed)
added 21 commits
-
3f5b0659...2d956eda - 20 commits from branch
master
- 7a78ea9f - Add update iteration cadence GraphQL mutation
-
3f5b0659...2d956eda - 20 commits from branch
removed review request for @krasio
- Resolved by Mario Celi
- Resolved by Mario Celi
- Resolved by Mario Celi
- Resolved by Mario Celi
- Resolved by Mario Celi
@mcelicalderonG great job, left a few small comments. I've also raised a question about how we would handle updates for the cases when a user downgrades from Premium to Starter in regards to multiple vs single cadences: #293864 (comment 526082401), but I guess we can also address that in a follow-up MR.
removed review request for @acroitor
removed database label
removed databasereview pending label
added 201 commits
-
4d477695...51a27920 - 200 commits from branch
master
- d0aea1d7 - Add update iteration cadence GraphQL mutation
-
4d477695...51a27920 - 200 commits from branch
@acroitor this one is ready for another look. Yes, I saw your restriction on the create mutation based on the cadence count for a group. I guess that there's a lot to discuss on how to handle that as I saw on your comment. At the update level I guess the only thing we could do is prevent updates to any cadence if the group exceeds the count, but actually having more than one cadence in the DB shouldn't be what enables them them to work with other parts of the code.