(Part 4) FE multiple approval rules - setup for mr
What does this MR do?
The components for MR create/edit and project settings share a lot of functionality. This MR updates the existing FE components for their reuse in MR create/edit.
Please note! This is part of a larger MR. You can see the full functionality here https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9168
Instructions!
- Please note that this does not point to
master
. - Do not merge until the related CE MR's are merged and the corresponding commits have been rebased out.
What are the relevant issue numbers?
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
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.7
added Create [DEPRECATED] Deliverable GitLab Premium approvals backend customer customer+ devopscreate direction frontend merge requests missed-deliverable missed:11.7 priority1 ~2975006 workflowin dev + 1 deleted label
removed missed-deliverable missed:11.7 labels
removed workflowin dev label
removed backend label
added 5 commits
Toggle commit list4 Warnings Most of the time, merge requests should target master
. Otherwise, please set the relevantPick into X.Y
label.This merge request is quite big (more than 858 lines changed), please consider splitting it into multiple merge requests. CHANGELOG missing. You can create one with:
bin/changelog -m 9176 "(Part 4) FE multiple approval rules - setup for mr"
If your merge request doesn’t warrant a CHANGELOG entry,
consider adding any of the ~backstage, ci-build, ~Documentation, meta, QA, test labels.
See the documentation.This merge request changed files with disabled eslint rules. Please consider fixing them. Disabled eslint rules
The following files have disabled
eslint
rules. Please consider fixing them:ee/app/assets/javascripts/pages/projects/edit/index.js
Run the following command for more details
node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \ 'ee/app/assets/javascripts/pages/projects/edit/index.js'
Generated by
DangerEdited by 🤖 GitLab Bot 🤖- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
- Resolved by Kushal Pandya
Hey @kushalpandya! Could you review and merge this next part of the "Multiple blocking approval rules"? I still need to create the CE MRs for this. I'm also working on breaking out my big branch into parts, so more will come soon
assigned to @kushalpandya
mentioned in merge request !9001 (merged)
mentioned in merge request !9201 (closed)
added 6 commits
-
792d9198 - 1 commit from branch
1979-fe-multiple-approval-rules
- b0d807d9 - Handle emptyText in user_avatar_list
- e4c28746 - Export default in vuex_shared/modal/actions
- 384dc17c - Extract modules from approvals store
- 779ef479 - Update approvals components to support MR use
- 7a87e9fa - Rename approvals index
Toggle commit list-
792d9198 - 1 commit from branch
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
@pslaughter I've done initial review and left some questions. Also, I believe some of the changes in this MR were already implemented in different CE MRs which I reviewed earlier, correct?
assigned to @pslaughter
Thanks so much @kushalpandya! I'll get this back to you soon
Also, I believe some of the changes in this MR were already implemented in different CE MRs which I reviewed earlier, correct?
I abstracted some common functionality and did some renaming. That's why some changes might have popped up again.
Edited by Paul Slaughteradded 2 commits
Thanks for the comments @kushalpandya! Back to you
assigned to @kushalpandya
Thanks @pslaughter, the MR looks good to me, also thanks for putting inline comments to explain context of change! Although if you think there are places in code where putting block comments for explaining context would make more sense then please do so as then we'd have context be always present.
Is this ready for merge?
assigned to @pslaughter
added 625 commits
-
30d1414d...04b316e3 - 622 commits from branch
1979-fe-multiple-approval-rules
- 09efacdc - Extract modules from approvals store
- 28d1dddf - Update approvals components to support MR use
- 7a5826f7 - Rename approvals index
Toggle commit list-
30d1414d...04b316e3 - 622 commits from branch
Although if you think there are places in code where putting block comments for explaining context would make more sense then please do so as then we'd have context be always present.
Sounds good!
Is this ready for merge?
Let's do it! I saw this was causing some FE specific pipeline issues so I:
- Fixed
gettext:lint
issues - Fixed
prettier
update issues - Fixed a
karma
issue withapprovers_list_item_spec.js
- Fixed
assigned to @kushalpandya
There's actually another
karma
issue on some unrelated specs (e.g.ee/spec/javascripts/operations/components/dashboard/project_header_spec.js
). This is because these specs use thegetChildInstances
function which is incompatible with@vue/test-utils
(this phenomenon is related to https://gitlab.com/gitlab-org/gitlab-ce/issues/56317). I'll resolve this in a follow up CE MR.mentioned in commit f7ea0b87
mentioned in merge request gitlab-com/www-gitlab-com!21373 (merged)
added Enterprise Edition label