Skip to content
Snippets Groups Projects

Validation to prevent default project, group visibility being restricted

Merged Roy Liu requested to merge 415524-adjust-admin-group-visibility-settings-on-saas into master
All threads resolved!

What does this MR do and why?

Related to #415524 (closed), on SaaS, user registration go through the Create or import your first project page, when either one of the default_group_visibility and default_project_visibility is restricted, user sees the following errors:

Can not create group Can not create project
group_error project_error

Screenshots or screen recordings

  • Example setting that triggers the validation error

Screenshot_2023-07-05_at_3.05.31_PM

  • Validation error message

Screenshot_2023-07-05_at_3.19.07_PM

How to set up and validate locally

  1. Simulate SaaS
  2. Turn on the feature flag by going to rails console and run Feature.enable(:prevent_visibility_restriction)
  3. Log in as admin
  4. Go to /admin/application_settings/general#js-visibility-settings
  5. Select Internal under Default group visibility
  6. Check Internal under Restricted visibility levels
  7. Submit the form

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Roy Liu

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
  • Roy Liu changed milestone to %Backlog

    changed milestone to %Backlog

  • Roy Liu assigned to @rliu-int

    assigned to @rliu-int

  • Roy Liu added 1 commit

    added 1 commit

    • 428af8a5 - Disable default group visibility level option in restricted visibility levels setting

    Compare with previous version

  • A deleted user added backend frontend labels

    added backend frontend labels

  • Contributor
    1 Warning
    :warning: Please add a merge request subtype to this merge request.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    backend Sylvester Chin current availability (@schin1) (UTC+8) Matthias Käppler current availability (@mkaeppler) (UTC+2)
    test for spec/features/* Sylvester Chin current availability (@schin1) (UTC+8) Maintainer review is optional for test for spec/features/*

    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.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Roy Liu added 1 commit

    added 1 commit

    • 5bddfebe - Disable default group visibility level option in restricted visibility levels setting

    Compare with previous version

  • Contributor

    Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 628f61a1 and 5bddfebe

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 4.13 MB 4.13 MB - 0.0 %
    mainChunk 2.99 MB 2.99 MB - 0.0 %

    Note: We do not have exact data for 628f61a1. So we have used data from: 0652bbbe.
    The intended commit has no webpack pipeline, so we chose the last commit with one before it.

    Please look at the full report for more details


    Read more about how this report works.

    Generated by :no_entry_sign: Danger

  • Roy Liu changed the description

    changed the description

  • Roy Liu mentioned in issue #415524 (closed)

    mentioned in issue #415524 (closed)

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for aad73efa

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Verify      | 8      | 0      | 0       | 8     | 8     | ❗     |
    | Data Stores | 20     | 0      | 0       | 15    | 20    | ❗     |
    | Govern      | 19     | 0      | 0       | 18    | 19    | ❗     |
    | Create      | 19     | 0      | 0       | 18    | 19    | ❗     |
    | Plan        | 47     | 0      | 0       | 40    | 47    | ❗     |
    | Manage      | 12     | 0      | 1       | 12    | 13    | ❗     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 125    | 0      | 1       | 111   | 126   | ❗     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-review-qa: :exclamation: test report for 5bddfebe

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Create           | 8      | 0      | 1       | 1     | 9     | ❗     |
    | Plan             | 3      | 0      | 1       | 0     | 4     | ✅     |
    | Manage           | 1      | 0      | 0       | 0     | 1     | ✅     |
    | Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Data Stores      | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 20     | 0      | 3       | 1     | 23    | ❗     |
    +------------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :heavy_minus_sign: test report for aad73efa

    expand test summary
    +-------------------------------------------------------------+
    |                       suites summary                        |
    +--------+--------+--------+---------+-------+-------+--------+
    |        | passed | failed | skipped | flaky | total | result |
    +--------+--------+--------+---------+-------+-------+--------+
    | Growth | 0      | 0      | 4       | 0     | 4     | ➖     |
    +--------+--------+--------+---------+-------+-------+--------+
    | Total  | 0      | 0      | 4       | 0     | 4     | ➖     |
    +--------+--------+--------+---------+-------+-------+--------+
  • Doug Stull
  • Roy Liu added 1 commit

    added 1 commit

    • 515ef29a - Disable default group visibility level option in restricted visibility levels setting

    Compare with previous version

  • Roy Liu added 1 commit

    added 1 commit

    Compare with previous version

  • Roy Liu changed the description

    changed the description

  • Doug Stull
  • Roy Liu added 1 commit

    added 1 commit

    Compare with previous version

  • Thong Kuah
  • Doug Stull
  • Doug Stull
  • Roy Liu added 1 commit

    added 1 commit

    Compare with previous version

  • Roy Liu added 1 commit

    added 1 commit

    • 9e53b5d8 - Disable default group visibility level option in restricted visibility levels setting

    Compare with previous version

  • Roy Liu requested review from @dstull

    requested review from @dstull

  • Roy Liu marked this merge request as ready

    marked this merge request as ready

  • Doug Stull
  • Doug Stull removed review request for @dstull

    removed review request for @dstull

  • Roy Liu added 1 commit

    added 1 commit

    • 30aeef3d - Add validation for default_group_visility when on saas

    Compare with previous version

  • Roy Liu requested review from @dstull

    requested review from @dstull

  • Doug Stull approved this merge request

    approved this merge request

  • :wave: @dstull, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • Doug Stull requested review from @tkuah and removed review request for @dstull

    requested review from @tkuah and removed review request for @dstull

  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • Roy Liu added 1404 commits

    added 1404 commits

    Compare with previous version

  • Roy Liu changed the description

    changed the description

    • Author Developer
      Resolved by Thong Kuah

      @tkuah I think from the discussions here, we can agree on that a fix is needed to prevent the conflicting settings. And we should go for fixing it for both SM and SaaS. I can progress following your suggestions here:

      1. With that in mind I suggest we release this validation behind a development feature flag but in Core
      2. I also think Doug's idea of only validating if values have changed is a good idea.
      3. Also potentially we can skip this validation when all levels have been restricted.

      Just have couple questions:

      1. Could you explain a bit on why put it behind a FF?
      2. I assume your third point is to protect SM instances that have all levels restricted, but if we switch to validate only on change, should that be enough?
  • Roy Liu mentioned in issue #417192 (closed)

    mentioned in issue #417192 (closed)

  • Roy Liu changed milestone to %16.2

    changed milestone to %16.2

  • Roy Liu changed the description

    changed the description

  • Roy Liu added 584 commits

    added 584 commits

    Compare with previous version

  • Roy Liu changed title from Adjust admin group visibility level settings on SaaS to Add validation preventing default group visibility being restricted

    changed title from Adjust admin group visibility level settings on SaaS to Add validation preventing default group visibility being restricted

  • Roy Liu added 1354 commits

    added 1354 commits

    Compare with previous version

  • Roy Liu changed title from Add validation preventing default group visibility being restricted to Add validation preventing default project, group visibility being restricted

    changed title from Add validation preventing default group visibility being restricted to Add validation preventing default project, group visibility being restricted

  • Roy Liu changed title from Add validation preventing default project, group visibility being restricted to Validation to prevent default project, group visibility being restricted

    changed title from Add validation preventing default project, group visibility being restricted to Validation to prevent default project, group visibility being restricted

  • Roy Liu added 1 commit

    added 1 commit

    • 1d095acc - Add validation to prevent visibility restriction

    Compare with previous version

  • A deleted user added feature flag label

    added feature flag label

  • Roy Liu added 1 commit

    added 1 commit

    • ced17840 - Add validation to prevent visibility restriction

    Compare with previous version

  • Roy Liu added 1 commit

    added 1 commit

    • 2871df23 - Add validation to prevent visibility restriction

    Compare with previous version

  • Roy Liu added 1 commit

    added 1 commit

    • db64fd38 - Add validation to prevent visibility restriction

    Compare with previous version

  • Roy Liu changed milestone to %16.3

    changed milestone to %16.3

  • Roy Liu changed the description

    changed the description

  • Author Developer

    @tkuah I updated this MR to add validations for default project visibility and default group visibility behind a feature flag. Would you mind giving this another review?

  • Roy Liu requested review from @tkuah

    requested review from @tkuah

    • Resolved by Roy Liu

      After a form validation error, the Default group visibility displays with nothing selected.

      I cannot select anything for Default group visibility after this. The only option is a reload of the page.

      Screenshot_2023-07-20_at_12-25-43_General___Admin_Area___GitLab

  • Thong Kuah
  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • Roy Liu added 1 commit

    added 1 commit

    • 52bbd81a - Add validation to prevent visibility restriction

    Compare with previous version

  • Roy Liu mentioned in issue #419316 (closed)

    mentioned in issue #419316 (closed)

  • Roy Liu requested review from @tkuah

    requested review from @tkuah

  • Thong Kuah
  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • Roy Liu added 1 commit

    added 1 commit

    • d7da93fd - Add test covering case when ff is off

    Compare with previous version

  • Roy Liu requested review from @tkuah

    requested review from @tkuah

  • Roy Liu added 1 commit

    added 1 commit

    • aad73efa - Add test covering case when ff is off

    Compare with previous version

  • Roy Liu changed the description

    changed the description

  • Thong Kuah resolved all threads

    resolved all threads

  • Thong Kuah approved this merge request

    approved this merge request

  • Thong Kuah enabled an automatic merge when the pipeline for c846f94e succeeds

    enabled an automatic merge when the pipeline for c846f94e succeeds

  • Great ! Thanks for working on this @rliu-int

  • merged

  • Thong Kuah mentioned in commit 2d3cdde2

    mentioned in commit 2d3cdde2

  • added workflowproduction label and removed workflowcanary label

  • Roy Liu mentioned in merge request !131203 (merged)

    mentioned in merge request !131203 (merged)

  • Roy Liu mentioned in issue #433280 (closed)

    mentioned in issue #433280 (closed)

  • Roy Liu mentioned in merge request !138370 (merged)

    mentioned in merge request !138370 (merged)

  • Please register or sign in to reply
    Loading