Skip to content
Snippets Groups Projects

(Part 4) FE multiple approval rules - setup for mr

All threads resolved!

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!

What are the relevant issue numbers?

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
  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter
  • Paul Slaughter added 3 commits

    added 3 commits

    • 929f4aa7 - Extract modules from approvals store
    • 7e016b0e - Update approvals components to support MR use
    • 6b230901 - Rename approvals index

    Compare with previous version

  • Author Maintainer

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

  • Paul Slaughter changed the description

    changed the description

  • Paul Slaughter mentioned in merge request !9001 (merged)

    mentioned in merge request !9001 (merged)

  • Paul Slaughter changed the description

    changed the description

  • Paul Slaughter mentioned in merge request !9201 (closed)

    mentioned in merge request !9201 (closed)

  • Paul Slaughter added 6 commits

    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

    Compare with previous version

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

  • Author Maintainer

    Thanks so much @kushalpandya! I'll get this back to you soon :smile:

    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 Slaughter
  • Paul Slaughter added 2 commits

    added 2 commits

    • a9974f49 - Update approvals components to support MR use
    • 5cb0ea83 - Rename approvals index

    Compare with previous version

  • Paul Slaughter added 3 commits

    added 3 commits

    • 90aa321d - Extract modules from approvals store
    • bb0a1ff9 - Update approvals components to support MR use
    • 30d1414d - Rename approvals index

    Compare with previous version

  • Author Maintainer

    Thanks for the comments @kushalpandya! Back to you :soccer:

  • Kushal Pandya approved this merge request

    approved this merge request

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

    Is this ready for merge?

  • Paul Slaughter added 625 commits

    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

    Compare with previous version

  • Paul Slaughter added 4 commits

    added 4 commits

    Compare with previous version

  • Author Maintainer

    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. :slight_smile:

    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 with approvers_list_item_spec.js
  • Author Maintainer

    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 the getChildInstances 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.

  • There are still some pipeline failures, should I merge it still? I know this MR targets another branch so I think we're fine but still confirming. :slight_smile:

  • Author Maintainer

    Let's do it. There's some failures BE needs to resolve. The FE one, I'll resolve in an upcoming MR.

  • Kushal Pandya resolved all discussions

    resolved all discussions

  • merged

  • Kushal Pandya mentioned in commit f7ea0b87

    mentioned in commit f7ea0b87

  • Please register or sign in to reply
    Loading