Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change authorized applications page #17656

Merged
merged 3 commits into from
Mar 1, 2022
Merged

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Feb 27, 2022

Start tracking when an OAuth access token was last used and from which IP (retain IP data only as long as our general IP retention period allows). Change the design of the authorized applications page in account settings to be more helpful. Change how application permissions are presented.

image

image

@Gargron Gargron force-pushed the feature-authorized-apps-redesign branch from 0e2bc59 to e58871a Compare February 27, 2022 23:52
@Gargron Gargron marked this pull request as ready for review February 27, 2022 23:53
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a full review as I have not read the parsing code yet, but I like the idea and the redesign, although I have some comments on the design itself:

  • I'm afraid it may take a lot of space per app, but I guess it's manageable
  • I think it would be better if there was a clearer visual separation between apps. They don't need to be in the same table.

%strong.announcements-list__item__title
= application.name
- if application.superapp?
%span.account-role.moderator= t('doorkeeper.authorized_applications.index.superapp')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not new but we probably shouldn't repurpose all those classes… either rename them to something more neutral or have the styles apply to different classes.

app/views/oauth/authorized_applications/index.html.haml Outdated Show resolved Hide resolved
db/schema.rb Outdated Show resolved Hide resolved
app/helpers/application_helper.rb Show resolved Hide resolved
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design-wise, I'd largely prefer if applications were each a different table / at least more visually separated.

Code-wise, I don't like that we are repurposing CSS classes which are now ill-named, but I can't hold that against this PR specifically, this is something that'll have to be addressed separately.

Otherwise this looks good to me!

@tribela
Copy link
Contributor

tribela commented Mar 1, 2022

Last used timestamp would very helpful if user want to get rid of old sessions
👍🏼

@Gargron Gargron merged commit 50ea54b into main Mar 1, 2022
@Gargron Gargron deleted the feature-authorized-apps-redesign branch March 1, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants