Skip to content
Snippets Groups Projects

Update iteration cadence mutation [RUN ALL RSPEC] [RUN AS-IF-FOSS]

Merged Mario Celi requested to merge update-iteration-cadence-mutation into master

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

Availability and Testing

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
Edited by Mario Celi

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • removed auto updated label

  • Mario Celi added 1 commit

    added 1 commit

    Compare with previous version

  • Mario Celi added 470 commits

    added 470 commits

    Compare with previous version

  • Mario Celi added 2 commits

    added 2 commits

    Compare with previous version

  • Mario Celi added 2 commits

    added 2 commits

    Compare with previous version

  • 🤖 GitLab Bot 🤖 changed title from WIP: Update iteration cadence mutation to WIP: Update iteration cadence mutation [RUN ALL RSPEC] [RUN AS-IF-FOSS]

    changed title from WIP: Update iteration cadence mutation to WIP: Update iteration cadence mutation [RUN ALL RSPEC] [RUN AS-IF-FOSS]

  • 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:

    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 🚫 Danger

    Edited by 🤖 GitLab Bot 🤖
  • Mario Celi added 1 commit

    added 1 commit

    • ad97f743 - Iteration cadences update mutation

    Compare with previous version

    • Author Maintainer
      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!

  • Mario Celi added 1 commit

    added 1 commit

    • c167dbc2 - Iteration cadences update mutation

    Compare with previous version

  • Mario Celi added 284 commits

    added 284 commits

    Compare with previous version

  • Mario Celi added 1 commit

    added 1 commit

    • c375757e - Iteration cadences update mutation

    Compare with previous version

    • Author Maintainer
      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:

      1. 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).
      2. 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.
      3. 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:

      1. Iterations feature is available for the group
      2. 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!

  • Mario Celi requested review from @acroitor

    requested review from @acroitor

  • Mario Celi added 1 commit

    added 1 commit

    • 16e79053 - Iteration cadences update mutation

    Compare with previous version

  • Mario Celi added 1 commit

    added 1 commit

    • cfbdc913 - Iteration cadences update mutation

    Compare with previous version

  • Mario Celi added 743 commits

    added 743 commits

    Compare with previous version

  • Mario Celi marked this merge request as ready

    marked this merge request as ready

  • Mario Celi changed milestone to %13.10

    changed milestone to %13.10

  • Mario Celi added 32 commits

    added 32 commits

    Compare with previous version

  • Mario Celi requested review from @krasio

    requested review from @krasio

  • mentioned in issue #322742 (closed)

  • Mario Celi added 21 commits

    added 21 commits

    Compare with previous version

  • Krasimir Angelov removed review request for @krasio

    removed review request for @krasio

  • @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.

  • Alexandru Croitor removed review request for @acroitor

    removed review request for @acroitor

  • removed database label

  • Mario Celi added 1 commit

    added 1 commit

    • 4d477695 - Apply 4 suggestion(s) to 1 file(s)

    Compare with previous version

  • Mario Celi added 201 commits

    added 201 commits

    Compare with previous version

  • Author Maintainer

    @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.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading