Make scopes in application OAuth screen collapsible

added pipelinetier-1 label

Thanks for your contribution to GitLab @abh80!

If you need help, page a coach by clicking here or come say hi on Discord.
When you're ready, request a review by clicking here.
We welcome AI-generated contributions! Read more/check the box at the top of the merge request description.
To add labels to your merge request, comment @gitlab-bot label ~"label1" ~"label2".

This message was generated automatically. You're welcome to improve it.

added Community contribution workflowin dev labels

assigned to @abh80

changed the description

This merge request will be counted as part of the running Hackathon!

Find yourself in the Hackathon Leaderboard or check out the Hackathon page for more information!

This message was generated automatically. You're welcome to improve it.

added Hackathon label

@gitlab-bot label frontend UX

added UX frontend labels

Thanks for helping us improve the UX of GitLab. Your contribution is appreciated! We have pinged our UX team, so stay tuned for their feedback.

This message was generated automatically. You're welcome to improve it.

added linked-issue label

	4 Warnings
	fd3ded1c: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
	138130bc: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
	featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart. For more information, see: The Handbook page on merge request types. The definition of done documentation.
	Labels missing: please ask a reviewer or maintainer to add backend to this merge request.

1 Message

CHANGELOG missing:

If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Category	Reviewer	Maintainer
backend	`@mkhalifa3` (UTC+2)	`@ahuntsman` (UTC-5)
frontend	`@jrushford` (UTC+2)	`@aturinske` (UTC-7)
UX	`@ipelaez1` (UTC-4)	Maintainer review is optional for UX
groupauthentication	Reviewer review is optional for groupauthentication	`@sgarg_gitlab` (UTC+5.5)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

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

Generated by Danger

mentioned in issue gitlab-org/quality/triage-reports#19481 (closed)

changed milestone to %Backlog

added Category:Permissions UI polish devopsgovern featureenhancement groupauthentication priority4 quick win sectionsec severity4 typefeature labels

Pinging you @aregnery as you recently did some updates to the OAuth when I remember correctly

requested review from @aregnery

requested changes

@mvanremmerden I wanted to get your opinion on the spacing. I was playing a bit with the padding and margin to get it to feel less spaced out, evenly spaced, and aligned. Question: What do you think about this? austins_changes.patch

Current	Proposed

Note: I saw you had the subtext with no padding, but I think it's visually easier to scan when aligned with the text in the button.

Current	Proposed

added 1 commit

6cbfc604 - Apply @aregnery patch

Compare with previous version

requested review from @aregnery

approved this merge request

added pipeline:mr-approved label

added pipelinetier-2 label and removed pipelinetier-1 label

Before you set this MR to auto-merge

This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3.

Before you set this MR to auto-merge, please check the following:

You are the last maintainer of this merge request
The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
This pipeline is recent enough (created in the last 8 hours)

If all the criteria above apply, please set auto-merge for this merge request.

See pipeline tiers and merging a merge request for more details.

changed the description

resolved all threads

@nradina will you give this a frontend review?

requested review from @nradina

@tvellishetty will you do the backend review?

requested review from @tvellishetty

question: @aregnery when testing in local I see a blue margin around the accordian item in focus, which is not present in the designs in the related issue. Is this blue margin expected?

mentioned in issue #467392 (closed)

Hi @abh80! Thank you for this great contribution.

I am awaiting the reply from the design team on how the expanded state should look like because I miss it in the issue description.

Also I noticed, that specs are missing for the new functionality and some of the existing features experienced unwanted changes.

Could you please have a look into my comments?

Back to you

requested changes

added 1 commit

665f9b3a - Update test and fix margin issue

Compare with previous version

requested review from @nradina

@aregnery @abh80 It seems like we are starting to see more and more challenges with the accordion component for this case, as it was neither designed nor written for this use case.

I played a bit around with the design, how about we move into a different direction and use popovers?

Doing it like this would give us the same design solution and avoid all questions like focus states, as well as implementation problems, and reduce the necessary changes to just one file. What do you two think?

If this is the direction we want to move towards, I have started to draft version already complete for the app/views/doorkeeper/authorizations/new.html.haml file:

.gl-ml-auto.gl-mr-auto{ class: 'sm:gl-w-1/2' }
  .gl-text-center.gl-flex.gl-flex-col.gl-items-center
    .gl-text-size-h1
      = html_escape(_('%{client_name} is requesting access to your account on %{title}.')) % { title: brand_title.html_safe, client_name: "<strong>#{html_escape(@pre_auth.client.name)}</strong>".html_safe }
    .gl-flex.gl-items-center.gl-gap-2.gl-pt-3.gl-pb-6
      = render Pajamas::AvatarComponent.new(current_user, size: 24, avatar_options: { data: { testid: 'user_avatar_content' }, title: current_user.username })
      .gl-pl-1
        %strong= current_user.name
      &middot;
      .gl-text-gray-500
        %span= current_user.to_reference
  - if current_user.admin?
    = render Pajamas::AlertComponent.new(variant: :warning, dismissible: false, alert_options: { class: 'gl-mb-5'}) do |c|
      - c.with_body do
        = html_escape(_('You are an administrator, which means authorizing access to %{client_name} will allow it to interact with GitLab as an administrator as well.')) % { client_name: "<strong>#{html_escape(@pre_auth.client.name)}</strong>".html_safe }
  - if @pre_auth.scopes
    %strong.gl-text-lg= _('Authorizing will allow this application to:')
    %div.gl-mb-5
      - @pre_auth.scopes.each do |scope|
        .gl-flex.gl-py-5.gl-gap-3.gl-border-b
          - title = t scope, scope: [:doorkeeper, :scopes]
          - content= t scope, scope: [:doorkeeper, :scope_desc]
          = title
          %span{ data: { toggle: 'popover', triggers: 'hover', html: 'true', placement: 'top', content: content }}
            = sprite_icon('information-o', css_class: 'gl-text-blue-500 hover:gl-text-blue-800 gl-cursor-pointer')
  .info-well
    .well-segment
      - if Gitlab.com? && !@pre_auth.client.application.owner
        %p.gl-text-green-500
          = sprite_icon('tanuki-verified')
          = _('This application is provided by GitLab.')
      - else
        %p.gl-text-orange-500
          = sprite_icon('warning-solid')
          = html_escape(_('Make sure you trust %{client_name} before authorizing.')) % { client_name: "<strong>#{html_escape(@pre_auth.client.name)}</strong>".html_safe }
      %p
        = html_escape(_('%{owner} %{created_date} ago.')) % { owner: auth_app_owner_text(@pre_auth.client.application.owner), created_date: time_ago_in_words(@pre_auth.client.application.created_at.to_date) }
        - domain = URI.parse(@pre_auth.redirect_uri).host.gsub('www.', '')
        - if @pre_auth.redirect_uri.start_with?('http://', 'https://') && domain != 'localhost'
          = html_escape(_('You will be redirected to %{domain} after authorizing.')) % { domain: "<strong>#{domain}</strong>".html_safe }
  %div
    = form_tag oauth_authorization_path, method: :post, class: 'gl-inline-block gl-pr-3' do
      = hidden_field_tag :client_id, @pre_auth.client.uid
      = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri
      = hidden_field_tag :state, @pre_auth.state
      = hidden_field_tag :response_type, @pre_auth.response_type
      = hidden_field_tag :scope, @pre_auth.scope
      = hidden_field_tag :nonce, @pre_auth.nonce
      = hidden_field_tag :code_challenge, @pre_auth.code_challenge
      = hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method
      = render Pajamas::ButtonComponent.new(type: :submit,
        variant: :confirm,
        button_options: {testid: 'authorization-button'}) do
        = html_escape(_('Authorize %{client_name}')) % { client_name: @pre_auth.client.name.html_safe }
    = form_tag oauth_authorization_path, method: :delete, class: 'gl-inline-block' do
      = hidden_field_tag :client_id, @pre_auth.client.uid
      = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri
      = hidden_field_tag :state, @pre_auth.state
      = hidden_field_tag :response_type, @pre_auth.response_type
      = hidden_field_tag :scope, @pre_auth.scope
      = hidden_field_tag :nonce, @pre_auth.nonce
      = hidden_field_tag :code_challenge, @pre_auth.code_challenge
      = hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method
      = render Pajamas::ButtonComponent.new(type: :submit) do
        = _('Cancel')

unapproved this merge request

approved this merge request

added 2 commits

f8530a5b - Added category option
138130bc - Add spec

Compare with previous version

@nradina @tvellishetty can we continue with merge if everything is good

added 1 commit

12d45f71 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

added 1 commit

fd3ded1c - Update _new_project_fields.html.haml

Compare with previous version

approved this merge request

requested review from @ahuntsman and removed review request for @tvellishetty

approved this merge request

removed review request for @ahuntsman

@aregnery can you please take a look at this to continue with merge

approved this merge request

resolved all threads

requested review from @eduardosanz

@abh80, great job! I left a few very minor comments.

requested changes

added 1 commit

3ab7faae - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

reset approvals from @ahuntsman by pushing to the branch

added 1 commit

236943e2 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

mentioned in issue gitlab-com/www-gitlab-com#34665 (closed)

removed review request for @nradina

closed

@abh80, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

React with a or a on this comment to describe your experience.
Create a new comment starting with @gitlab-bot feedback below, and leave any additional feedback you have for us in the comment.

As a benefit of being a GitLab Community Contributor, you can request access to GitLab Duo. With Code Suggestions, Chat and more AI-powered features, GitLab Duo helps to boost your efficiency and effectiveness by reducing the time required to write and understand code. Visit the Duo access project to request a GitLab Duo license and learn more about the benefits of GitLab Duo.

Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.

Thanks for your help!

This message was generated automatically. You're welcome to improve it.

@abh80, did you close this MR by mistake? Please, could we reopen this MR. This is a valuable contribution that we would like to merge.

I have been away on a conference, apologies that I haven't been able to provide a faster review.

reopened

added 1 commit

670268f5 - Simplify the code to achieve fairly similar result

Compare with previous version

added backend label

3 Warnings

: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.

featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

For more information, see:

The Handbook page on merge request types.
The definition of done documentation.

Reviewer roulette

Category	Reviewer	Maintainer
backend	`@emeraldjayde` (UTC+0)	`@ahuntsman` (UTC-5)
frontend	`@ccharnolevsky` (UTC+3)	`@sdejonge` (UTC+11)
UX	`@ipelaez1` (UTC-4)	Maintainer review is optional for UX
groupauthentication	Reviewer review is optional for groupauthentication	`@atevans` (UTC-7)

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

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

Generated by Danger

added 1 commit

40d16882 - Simplify the code to achieve fairly similar result

Compare with previous version

@abh80, I have pushed one commit and reopened the MR. Thank you very much for your work! We hope you continue contributing!

@nradina, please could you re-review this MR? Thanks!

resolved all threads

requested review from @nradina

removed review request for @eduardosanz

@dblessing, please could you review this? Thanks!

It is a frontend MR but because I have authored a commit I can't approve it.

requested review from @dblessing

Bundle size analysis [beta]

This compares changes in bundle size for entry points between the commits 4c1c7cdd and 02ace5bf

Special assets

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
average	4.39 MB	4.39 MB	-	0.0 %
mainChunk	3.3 MB	3.3 MB	-	0.0 %

Significant Growth: 2

Expand

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
pages.oauth	398 Bytes	1.71 KB	+1.32 KB	338.7 %
pages.oauth.applications	6.58 KB	7.9 KB	+1.32 KB	20.0 %

Your MR has at least one entrypoint growing significantly (more > 1 KB or 2%). If you write new or extend existing features, this is expected and there is nothing to worry about.

Please consider pinging someone from the FE Foundations (@leipert, @markrian, @pgascouvaillancourt, @sdejonge or @thutterer) for review, if you are unsure about the size increase.

Note: We do not have exact data for 4c1c7cdd. So we have used data from: bef6671a.
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

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for 02ace5bf

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Create      | 128    | 0      | 18      | 0     | 146   | ✅     |
| Govern      | 71     | 0      | 2       | 0     | 73    | ✅     |
| Verify      | 45     | 0      | 2       | 0     | 47    | ✅     |
| Plan        | 76     | 0      | 0       | 0     | 76    | ✅     |
| Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
| Manage      | 1      | 0      | 1       | 0     | 2     | ✅     |
| Secure      | 4      | 0      | 0       | 0     | 4     | ✅     |
| Package     | 17     | 0      | 18      | 0     | 35    | ✅     |
| Data Stores | 33     | 0      | 1       | 0     | 34    | ✅     |
| Fulfillment | 2      | 0      | 0       | 0     | 2     | ✅     |
| Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 392    | 0      | 42      | 0     | 434   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-test-on-cng: test report for 02ace5bf

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Package     | 24     | 0      | 14      | 0     | 38    | ✅     |
| Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
| Verify      | 50     | 0      | 15      | 10    | 65    | ✅     |
| Create      | 139    | 0      | 20      | 13    | 159   | ✅     |
| Plan        | 86     | 0      | 8       | 8     | 94    | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Govern      | 82     | 0      | 7       | 8     | 89    | ✅     |
| Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
| Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
| Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
| Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
| ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
| Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
| Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
| Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
| Secure      | 4      | 0      | 2       | 0     | 6     | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 436    | 0      | 113     | 39    | 549   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-test-on-omnibus: test report for 02ace5bf

expand test summary

+---------------------------------------------------------------------+
|                           suites summary                            |
+----------------+--------+--------+---------+-------+-------+--------+
|                | passed | failed | skipped | flaky | total | result |
+----------------+--------+--------+---------+-------+-------+--------+
| Create         | 423    | 0      | 61      | 0     | 484   | ✅     |
| Govern         | 108    | 0      | 6       | 0     | 114   | ✅     |
| Analytics      | 1      | 0      | 0       | 0     | 1     | ✅     |
| Systems        | 6      | 0      | 1       | 0     | 7     | ✅     |
| Package        | 7      | 0      | 0       | 0     | 7     | ✅     |
| Manage         | 25     | 0      | 10      | 6     | 35    | ✅     |
| GitLab Metrics | 2      | 0      | 1       | 0     | 3     | ✅     |
| Data Stores    | 13     | 0      | 1       | 0     | 14    | ✅     |
| Fulfillment    | 2      | 0      | 0       | 0     | 2     | ✅     |
| Secure         | 1      | 0      | 0       | 0     | 1     | ✅     |
| Configure      | 1      | 0      | 0       | 0     | 1     | ✅     |
| Plan           | 1      | 0      | 0       | 0     | 1     | ✅     |
| Monitor        | 4      | 0      | 1       | 0     | 5     | ✅     |
| Ai-powered     | 1      | 0      | 0       | 1     | 1     | ✅     |
+----------------+--------+--------+---------+-------+-------+--------+
| Total          | 595    | 0      | 81      | 7     | 676   | ✅     |
+----------------+--------+--------+---------+-------+-------+--------+

resolved all threads

assigned to @eduardosanz

enabled automatic add to merge train when checks pass

added 804 commits

40d16882...5f41988f - 793 commits from branch gitlab-org:master
55b24c50 - 1 earlier commit
ac9b0ca6 - Added accordion to oauth authorize page
ec08eeb9 - Apply @aregnery patch
52ee3421 - Update test and fix margin issue
e66d9abf - Added category option
6bf459fb - Add spec
5281151b - Apply 1 suggestion(s) to 1 file(s)
09b2625b - Update _new_project_fields.html.haml
c185593c - Apply 1 suggestion(s) to 1 file(s)
a1ec4ac6 - Apply 1 suggestion(s) to 1 file(s)
02ace5bf - Simplify the code to achieve fairly similar result

Compare with previous version

approved this merge request

added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-2 label

bypassed reviews on this merge request

enabled automatic add to merge train when checks pass

Note on why I bypassed requested changes. I believe the reason that happened is because Eduardo requested changes, but he subsequently committed to the MR and could no longer serve as approver. Yet the requested changes was still blocking the MR. Other requested changes were subsequently approved: Nataliia, Austin.