Peter Hegman (66b8a07b) at 19 Mar 01:37

What does this MR do and why?

Implement JSRoutes middleware, frontend tests, and docs

So the JavaScript path helpers can now be used in production code

References

Screenshots or screen recordings

Before	After

How to set up and validate locally

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Peter Hegman (8c3192b9) at 19 Mar 01:31

@tkuah okay thanks, yeah that makes sense! So sounds like we should remove web_url and web_path methods from the model and move them to a presenter layer and then enforce that with linting. Please correct if I am misunderstanding 🙂

@niskhakova hmm good question. I wasn't aware of Presenter::Base that defines web_url and web_path. This may be a bit out of my Rails knowledge. I guess the main question is "what layer will web_url and web_path be used?" 🤔. After doing some searching through the codebase it doesn't seem like we are using web_url in any controllers. It seems like they are only used in the "view" layer (HAML, GraphQL types, Grape entities). This makes me think that a presenter may be a better option than a concern.

---

I feel like we need to get a Rails experts opinion on the best path forward here though. @tkuah would you be available to help here or suggest someone to help? Here is the Tl;DR of what we are trying to accomplish:

Currently there is inconsistency around how we use/define the web_url and web_path methods. Sometimes they are defined in models, sometimes in presenters, and sometimes we directly call Gitlab::UrlBuilder.build(). The goal is align around a SSOT for web_url (absolute URL) and web_path (relative URL) and then build linters to enforce using this SSOT.

Do you have any thoughts on the best way to accomplish this? Thanks in advance!

@dblessing overall looks good, just a couple minor comments/questions but nothing blocking frontend review so I will approve 🙂

What does this MR do and why?

Follow-up to !225916, add sidebar menu items for organization admin area as well as a root controller to redirect a user to the correct dashboard. An organization owner is redirected to the organization dashboard and an instance admin/custom admin role users are redirected to the instance dashboard.

This feature is behind a feature flag added in the previous MR. This MR serves as a minimal framework to build new organization features or to migrate existing features from instance or group to organizations.

This MR looks large but there are lots of small changes to update spec path helpers. Separating into multiple MR would have left something untestable (sidebar present that leads to nowhere).

In a follow-up MR we will add content to the organization dashboard.

References

#587370

Screenshots or screen recordings

Before	After
Instance admin view	Instance admin view Organization owner view Very minimal for now. Will subsequently add user management views + organization settings

How to set up and validate locally

1. Enable Feature Flags. In Rails console (bundle exec rails c):

   Feature.enable(:org_admin_area)
   Feature.enable(:ui_for_organizations)
   Feature.enable(:organization_switcher)
   Feature.enable(:organization_scoped_paths)

2. Create a New Organization

   org = Organizations::Organization.create!(name: 'Test Organization', path: 'test-org')

3. Create or Find a User to Make Org Owner. Use existing or create new.

   Users::CreateService.new(User.first, { name: 'Org Owner', username: 'org_owner', email: '[email protected]', password: 'mysupersecretpassword!', organization_id: org.id, organization_access_level: :owner }).execute

4. Sign in as the org owner user (not an instance admin)

5. You should see the Admin button in the top right. Click the button and it should take you to the blank Organization Admin page.

6. Sign in as instance admin

7. Click the Admin button in the top right and observe admin area is accessible as per normal. But you will also notice that a new menu item Organization overview is present.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

suggestion: I think we need to change some documentation in https://docs.gitlab.com/administration/admin_area/ if we change this title. We could also add a feature flag check here for now? WDYT?

question (non-blocking): Why do we need these new routes? Could it just be /admin and /o/acme/admin?

@jblack7 @dblessing following up from our conversation today about "separate organization dashboard" vs "organization admin area".

Background

We originally went down the route of separate organization dashboard and moving all the settings over from the admin area to the organization dashboard. The main concern here was that moving the settings over would be difficult and we already had the admin area built that we could leverage. There is a lot of discussion in gitlab#554579 (comment 2688988848) and the decision was officially made in gitlab-com/content-sites/handbook!15583 (merged). I was initially against this because because we had put a lot of effort into the original plan but was convinced that it was a better path because of how much effort it would be to move the settings over. I do think one thing that may have been missed in that discussion/decision is "where do features for non-owners live?". I think the assumption was that they would be in "Your work" or "Explore".

Concerns about separate organization dashboard

My main concern is moving the admin settings over. This will be a huge effort and will also result in a long in-between stage where some settings or in the admin area and some settings are in the organization dashboard. Moving settings could also be considered a breaking change which will slow the process down.

Hybrid approach

Taking a hybrid approach could be a good option. All organization settings and user management are in the admin area (/o/acme/admin) and organization features that are available to all users (not just owners) are in the organization dashboard. My concern with this is that we end up with settings in multiple places and a confusing UX. We could prevent this with strict guidelines:

• Organization dashboard only contains features that are available to all users (not just owners)
• Anything that is only available to owners is in the admin area

We could also setup the sidebar in the organization dashboard to link to the admin area for owners. Something like this:

Peter Hegman (c8e6ecc4) at 18 Mar 17:44

... and 1 more commit

What does this MR do and why?

One dropdown in app/assets/javascripts/vue_shared/components/new_resource_dropdown/new_resource_dropdown.vue should be migrated to one of the new dropdown components.

References

Related Issue - #418039

Screenshots or screen recordings

Before	After

How to set up and validate locally

1. Go to this http://127.0.0.1:3000/groups/flightjs/-/issues to test locally.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #418039

@markrian can you help review this? Also related, interested to get your take on gitlab-org/gitlab-services/design.gitlab.com!5737 and duo-ui!477 in regards to if we should pin to 3.5.30 instead of ^3.5.30? 🤔

Peter Hegman (8cdb3129) at 18 Mar 17:04

Peter Hegman (f2034b7d) at 18 Mar 16:52

Peter Hegman (f2034b7d) at 18 Mar 16:52

... and 1 more commit

What does this MR do?

Related to gitlab!227698 and gitlab-org/gitlab-services/design.gitlab.com!5737. We want to keep Vue 3 versions in sync across gitlab-org/gitlab, gitlab-ui, and duo-ui. Duo UI was already on 3.5.30 but it used the ^ to indicate any minor or patch version could be used. This was out of sync with how gitlab-org/gitlab was setup so it is now pinned to 3.5.30.

For now keeping the Vue versions in sync is a manual process but I will look into updating Renovate to automate this.

Peter Hegman (ea08e7d6) at 18 Mar 16:49

This already has a backend reviewer assigned, removing myself

@poojaghanghas479 I think it was a flaky spec. Re-trying 🤞