Skip to content

Allauth: tune login/connect sorting, naming, and add GitHub App/Oauth modal #609

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 9, 2025


Work left:

  • Test with SAML providers
  • On or before release, all database SocialApps need settings in JSON field for {"hidden": true} and testing on actual auth.
    • Instead of relying on hidden, SAML providers were filtered from get_providers

Examples

Business with Google provider and GitHub modal button

image

Business when no modal is needed

image

GitHub modal

image

Social account list

image

agjohnson added 2 commits June 9, 2025 12:41
…hen two are visible

This makes use of Allauth's app settings, which already supports
`hidden`, and extends this to support provider sorting and tuning of
login/connect/etc process hiding of providers.
@agjohnson
Copy link
Contributor Author

In testing I also figured I'd fix #588

image

Can't decide if that should be labeled SAML or SAML for Someone, Inc. A long label would blow up that listing UI and it feels like this is information for core team and support, not for the user. Users of this SAML app aren't going to see SAML for Someone Else, LLC ever so why specify it verbosely?

agjohnson added 2 commits June 9, 2025 15:53
Show icon instead of missing avatar and use more helpful strings for the
listing
@agjohnson agjohnson marked this pull request as ready for review June 9, 2025 23:27
@agjohnson agjohnson requested review from a team as code owners June 9, 2025 23:27
@agjohnson agjohnson requested a review from stsewd June 9, 2025 23:27
@humitos
Copy link
Member

humitos commented Jun 10, 2025

I'm a little confused with this screenshot.

It seems that we are mixing the copy for the "Login / Signup" with the "Connect to GitHub". When I'm connecting my account, it shouldn't say "For users that connected their account before June". That doesn't make sense on that screen, but it does on "Login / Signup", tho.

453181577-81f390f1-f251-43c1-bce3-7f274cef8800

@agjohnson
Copy link
Contributor Author

Good point, we could use an extra step in this. The copy here now is for the case of:

  • Existing users reconect with OAuth
  • New users go to GHA

But I think what you're looking for is some copy for early beta that covers the case of:

  • Most users should use OAuth still
  • Only adventurous users should use GHA

There's definitely room for that here.

@stsewd
Copy link
Member

stsewd commented Jun 10, 2025

I thought we decided to start the beta with one button only, and only add two buttons if there is need for it?

@agjohnson
Copy link
Contributor Author

Connection page updates, including:

  • Style updates that weren't reflected in the description
  • Changed header text to drop "legacy"/"new". This doesn't feel like a good fit for initial beta, we can call OAuth "legacy" as we promote GHA status more though.
  • Added "Default" label to help communicate what we suggest better
  • Added early and late beta text on connect page

Connect view during early beta, guide users to stick with OAuth

image

Connect view during later beta, guide users to try GHA

Note: we might also swap the priority of these when we make the code change to trigger this copy, it feels like this copy would make sense with GHA promoted maybe. I didn't do that for this screenshot though.

image

@agjohnson
Copy link
Contributor Author

I thought we decided to start the beta with one button only, and only add two buttons if there is need for it?

Think we landed on the opposite, working towards a single button but giving the user explicit options especially early in the beta. We're bound to notice edge cases and benefit from giving users control.

This plan was laid out in #608

A single button might be safe enough, but we can wait for user testing to prove that. I don't have any worries about users being confused by this UI either way, it's quite explicit.

@stsewd
Copy link
Member

stsewd commented Jun 11, 2025

Think we landed on the opposite, working towards a single button but giving the user explicit options especially early in the beta. We're bound to notice edge cases and benefit from giving users control.
This plan was laid out in #608

That issue seems new? This is what I wrote just after the meeting https://github.com/readthedocs/meta/issues/187#issuecomment-2917068587.

A single button might be safe enough, but we can wait for user testing to prove that. I don't have any worries about users being confused by this UI either way, it's quite explicit.

Just to be clear, having two buttons doesn't allow users to "go back" to the previous state if they don't like something about the new app or if they encounter an error.

But I do like having the modal to get users into the beta, but I'd be still -1 on showing two buttons when the beta ends. We are asking users to remember if they migrated or not, or the date they created their account, which users won't have idea about.

I also wasn't able to run this locally, socialaccount/snippets/provider_list_item.html doesn't exist.

@agjohnson
Copy link
Contributor Author

This is what I wrote just after the meeting https://github.com/readthedocs/meta/issues/187#issuecomment-2917068587.

Not revoking access to OAuth on migration is still on plan, that's fine.

so they can still use the old GH button (no need to have two login options during the beta).

But here, we still want users trying to log in with GHA so we can test the whole workflow. We haven't tested this ourselves yet and we also want users testing the GHA buttons so we can confirm if we can reduce to a single button.

Just an OAuth button won't push much farther forward on testing and I think we just want to jump all in on testing.

What is the concern with two buttons at this point?

Just to be clear, having two buttons doesn't allow users to "go back" to the previous state if they don't like something about the new app or if they encounter an error.

Yeah I think at least initially it's more likely the user tries to connect their GitHub to GHA, hits an issue and can't connect and needs to fall back to OAuth, or stick with an existing OAuth connection.

I'd be still -1 on showing two buttons when the beta ends

This is definitely the hope, it's in the plan at #608

Now I also wouldn't say it's impossible that we have two buttons after beta either though. If we find users are absolutely blocked on migrating to GHA, we could still need this. But we deal with that if we deal with that.

@humitos
Copy link
Member

humitos commented Jun 17, 2025

I'd like to move forward with the GHA beta and deploy this feature. I'm reviewing all these PRs and it seems we are blocked on relevant things. However, I think we can tune them in the following deploys. We won't know too much about the UX of having one or two buttons until we deploy these changes and start taking to customers.

I personally prefer one button because two adds noise to me. However, the UI is explicit enough to make me feel more comfortable with it for now as long as we are flexible to change it again in the following deploys.

@stsewd do you have any other technical consideration here? Anything else that's blocking this PR or are you OK to move forward for now?

@agjohnson
Copy link
Contributor Author

And just to be clear, I also want only one button here, I just don't think we can start with one button without potentially upsetting customers. We'll know more as users test this whole workflow.

@stsewd
Copy link
Member

stsewd commented Jun 17, 2025

@stsewd do you have any other technical consideration here?

Nope

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

This is still showing a date by default

Screenshot 2025-06-19 at 18-53-39 Sign up - Read the Docs Dev

We could also link to the blog post.

@agjohnson agjohnson requested review from stsewd and humitos June 20, 2025 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants