Resolve "Add "No approval required" state to approval rules MR component"
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
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
EE specific content should be in the top level /ee
folder -
For a paid feature, have we considered GitLab.com plans, how it works for groups, and is there a design for promoting it to users who aren't on the correct plan? -
Security reports checked/validated by reviewer
Merge request reports
Activity
changed milestone to %11.9
@sbigelow, do you mind giving this a pre-maintainer review? Thanks so much!
assigned to @sbigelow
1 Warning The title of this merge request is longer than 72 characters and would violate our commit message rules when using the Squash on Merge feature. Please consider adjusting the title, or rebase the commits manually and don’t use Squash on Merge. 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 randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer frontend Enrique Alcántara ( @ealcantara
)Filipa Lacerda ( @filipa
)Generated by
DangerEdited by 🤖 GitLab Bot 🤖- Resolved by Paul Slaughter
- Resolved by Phil Hughes
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
@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 onthis.mr.approvals
if it is undefined.I could be off here, so they are just mentioned as questions.
assigned to @pslaughter
added 1 commit
- ef2a70ae - Add default approvals obj to rules mr component
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
Toggle commit list-
ef2a70ae...696f85b3 - 40 commits from branch
- Resolved by Phil Hughes
- Resolved by Phil Hughes
- Resolved by Phil Hughes
added 1 commit
- b60f3444 - Add default approvals obj to rules mr component
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!
assigned to @iamphill
- Resolved by Phil Hughes
assigned to @pslaughter
added 1 commit
- 3d462b4f - fixup! Add approvals summary optional component
marked as a Work In Progress from 3d462b4f
Thanks @iamphill! Back to you
assigned to @iamphill
enabled an automatic merge when the pipeline for d9ba6c77 succeeds
mentioned in commit 9dedf055
mentioned in issue gitlab-org/release/tasks#700 (closed)
mentioned in merge request !9952 (merged)
mentioned in commit 491cd60c
mentioned in commit release-tools@4cb0ef2a
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.
@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.
@jonas1 Thank you very much, please mention me on the issue, so it stays on my radar
.Done (#10750 (closed)) :)
p.s.: Hello fellow Belgian ;)
mentioned in issue #10750 (closed)
@pslaughter This merge request could not automatically be picked into
11-8-stable-ee
for11.8.8-ee
and will need manual intervention.@pslaughter This merge request could not automatically be picked into
11-8-stable-ee
for11.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.
mentioned in commit reprazent/gitlab-ee@90d76222
mentioned in merge request !11417 (closed)
mentioned in merge request !11411 (merged)
mentioned in merge request !11411 (merged)
mentioned in commit reprazent/gitlab-ee@ea4372ea
added Enterprise Edition label