Skip to content
Snippets Groups Projects

Resolve "Add "No approval required" state to approval rules MR component"

Merged Paul Slaughter requested to merge 9963-optional-approval-view into master
1 unresolved thread

What does this MR do?

The previous implementation of multiple blocking approval rules #1979 (closed) did not include the "No approvals required" view. This MR adds this feature to the new set of MR widget approvals components.

Please note:

To test this feature you will need to enable the feature flag approval_rules.

What are the relevant issue numbers?

Closes #9963 (closed)

Screenshots

Screen_Shot_2019-03-06_at_2.15.33_PM

Does this MR meet the acceptance criteria?

Edited by Paul Slaughter

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
  • Sam Bigelow
  • This looks good to me! :thumbsup: Just curious about the one empty spec.

  • @pslaughter I was thinking about this later and wanted to give it some more attention. At a better look, I basically just noticed some places that could use more null checking. This lead me to thinking it may be worth just a computed approvals property that could default values on this.mr.approvals if it is undefined.

    I could be off here, so they are just mentioned as questions.

  • Paul Slaughter added 3 commits

    added 3 commits

    • 93dbab4c - Add approvals summary optional component
    • 273f28f3 - Fix use of deprecated hasClass in spec
    • 07703c65 - Add default approvals obj to rules mr component

    Compare with previous version

  • Paul Slaughter added 1 commit

    added 1 commit

    • ef2a70ae - Add default approvals obj to rules mr component

    Compare with previous version

  • Paul Slaughter added 44 commits

    added 44 commits

    • ef2a70ae...696f85b3 - 40 commits from branch master
    • ee217fc0 - Move optional approval text to messages
    • 6797ac5f - Add approvals summary optional component
    • 1dceb389 - Fix use of deprecated hasClass in spec
    • ae6f428f - Add default approvals obj to rules mr component

    Compare with previous version

  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter added 1 commit

    added 1 commit

    • b60f3444 - Add default approvals obj to rules mr component

    Compare with previous version

  • Author Maintainer

    Thanks so much for the review @sbigelow! I really appreciate you diving into the null safeness of these nested props. Please see this comment https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9899/diffs#note_148100921 which describes my update.

    For the sake of time, I'll go ahead and pass this off to a maintainer. @iamphill, would you be available to review and merge this one? Thank you! :bow:

  • Phil Hughes resolved all discussions

    resolved all discussions

  • Phil Hughes
  • Paul Slaughter added 1 commit

    added 1 commit

    • 3d462b4f - fixup! Add approvals summary optional component

    Compare with previous version

  • Paul Slaughter marked as a Work In Progress from 3d462b4f

    marked as a Work In Progress from 3d462b4f

  • Paul Slaughter added 3 commits

    added 3 commits

    • 25da9014 - Add approvals summary optional component
    • f0d4a92e - Fix use of deprecated hasClass in spec
    • d8591854 - Add default approvals obj to rules mr component

    Compare with previous version

  • Paul Slaughter unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Author Maintainer

    Thanks @iamphill! Back to you :soccer:

  • Paul Slaughter added 3 commits

    added 3 commits

    • e7d936f5 - Add approvals summary optional component
    • b51bcb35 - Fix use of deprecated hasClass in spec
    • d9ba6c77 - Add default approvals obj to rules mr component

    Compare with previous version

  • Phil Hughes resolved all discussions

    resolved all discussions

  • Phil Hughes approved this merge request

    approved this merge request

  • Phil Hughes enabled an automatic merge when the pipeline for d9ba6c77 succeeds

    enabled an automatic merge when the pipeline for d9ba6c77 succeeds

  • merged

  • Phil Hughes mentioned in commit 9dedf055

    mentioned in commit 9dedf055

  • Mark Chao added 1 deleted label

    added 1 deleted label

  • This merge request could not automatically be picked into 11-8-stable-ee for 11.8.2-ee and will need manual intervention.

  • mentioned in merge request !9952 (merged)

  • Phil Hughes mentioned in commit 491cd60c

    mentioned in commit 491cd60c

  • Tracking things down it appears that this MR is now preventing core users from freely approving MR's.

    This has been possible for years and the Starter+ feature always seemed to be about the blocking/required nature of them, not freely doing them.

    regression

  • @jonas1 The functionality in Core/Free shouldn't have changed at all as there are no approvals. For Starter/Bronze customers, the new rules-feature should still allow the same functionality.

    The feature flag is off right now, so we would greatly appreciate any feedback you have regarding a possible regression. Would you mind opening an issue with steps to reproduce in https://gitlab.com/gitlab-org/gitlab-ee/issues/new ?

  • @reprazent From what I can see exactly what got changed in this MR:

    Before 11.8 Core/Free users could use the approval interface to add approvals to MR's

    Where Starter+ added restrictions related to it: In Core/Free approvals could be added but would never block an MR, and could thus not be enforced. This matches the "no approvals required" state (No approvals are required, as that's a premium feature, but you can still approve)

    This has been the case since I convinced the company I'm currently working to start using Gitlab.

  • Before 11.8 Core/Free users could use the approval interface to add approvals to MR's

    @jonas1 According to the docs that feature should only be available for Starter/Bronze. I've just tried that with a private project on GitLab.com and the approvals feature is correctly hidden.

    That seems to have been the case for 11.7 as well.

    The code for approvals shouldn't be present in the http://gitlab.com/gitlab-org/gitlab-ce codebase, except for the render_if_exists that hook it in. The table to store approvals is also missing from the CE-schema.

    Obviously, I'm missing something. The easiest way to figure this out, would be a new issue, with steps to reproduce. If you are seeing the regression on GitLab.com, you could even point us to an example project showing the regression.

  • The issue happens on an omnibus install (of -ee), I'll convert it into a new issue

  • mentioned in issue #10750 (closed)

  • @pslaughter This merge request could not automatically be picked into 11-8-stable-ee for 11.8.8-ee and will need manual intervention.

  • @pslaughter This merge request could not automatically be picked into 11-8-stable-ee for 11.8.8-ee and will need manual intervention.

  • This does not pick cleanly so we either need to remove it from 11.8 changeset or someone needs to create backports to stable branches for both ce and ee. @pslaughter or @reprazent, this is your call. Do let me know within the next 3 hours.

  • Bob Van Landuyt mentioned in merge request !11417 (closed)

    mentioned in merge request !11417 (closed)

  • mentioned in merge request !11411 (merged)

  • mentioned in merge request !11411 (merged)

  • Bob Van Landuyt removed 1 deleted label

    removed 1 deleted label

  • Please register or sign in to reply
    Loading