Add template and model view changes for organization authorization settings

[agjohnson]

This relies on https://github.com/readthedocs/readthedocs-corporate/pull/1730

This drops the very custom form for a more native form, but retains some of the styles of the authorization provider list. This is now just a informational block describing the providers.

This also adds the KO view logic back to the view, so that we can conditionally show/require the domain field.

Also raised in the PR above, a warning block is added when the provider value is changing, so there is some information that authentication/authorization might fail.

Screencast from 2024-02-16 11-34-38.webm

• Fixes #219

[agjohnson]

Test failure is https://github.com/CircleCI-Public/browser-tools-orb/issues/102

Tests pass locally:

Chrome: |██████████████████████████████| 4/4 test files | 11 passed, 0 failed

Finished running tests in 2.8s, all tests passed! 🎉

[agjohnson]

@stsewd based on your comment in #219, it wasn't clear to me what we want to do with the domain field yet. If this is read only, we probably want to handle this field differently at the Form level I suppose?

[humitos]

I didn't review the code, but the video looks great! 💯

This drops the very custom form for a more native form, but retains some of the styles of the authorization provider list. This is now just a informational block describing the providers.

I like how it looks. However, I'm always hesitant to manipulate forms directly from the template because it gets hard to maintain the HTML code pretty easily, but I don't have a better suggestion here, so 🤷🏼

This also adds the KO view logic back to the view, so that we can conditionally show/require the domain field.

If I understand correctly, this domain field should be removed since the user shouldn't write anything here. The user enabling Google SSO should have their Google account already connected and the domain should be populated from there only --for security reasons.

Also raised in the PR above, a warning block is added when the provider value is changing, so there is some information that authentication/authorization might fail.

This is 💯

[stsewd]

@stsewd based on your comment in https://github.com/readthedocs/ext-theme/issues/219, it wasn't clear to me what we want to do with the domain field yet. If this is read only, we probably want to handle this field differently at the Form level I suppose?

That field is read-only, to let the user know which domain is going to be used for the integration. The domain is restricted to domain from their current connected Google account. Currently, we allow linking several accounts of the same provider, so this could be a choice field in case there are several domains available.

[agjohnson]

gets hard to maintain the HTML code pretty easily, but I don't have a better suggestion here, so 🤷🏼

Yeah, I think this is a necessary evil for now. I am actively thinking about ways to make this better, but currently it's still annoying to use Django Form/crispy, Django templates, and Knockout simultaneously. Especially so any time there needs to be deeper control of the crispy field HTML structure, like this PR.

That field is read-only, to let the user know which domain is going to be used for the integration.

So, it seems like it would be best if the Form throws a validation error if the user doesn't have a connected Google account and Google is selected.

But on the domain field, at what point do we expect a value in this field? After the user submits this form I assume? If so, it might make sense to drop domain as a form field and instead show some extra content with the value above the form field -- similar to how the shareable link for authentication shows up for Allauth SSO on that form.

Does that make sense?

[stsewd]

But on the domain field, at what point do we expect a value in this field? After the user submits this form I assume? If so, it might make sense to drop domain as a form field and instead show some extra content with the value above the form field -- similar to how the shareable link for authentication shows up for Allauth SSO on that form.

I think it makes sense before, so the user knows the action he's about to take. Like showing "You are about to connect to your @readthedocs.com Google Workspace" or something, also, we should probably allow connecting to just one Google account.

[humitos]

@agjohnson @stsewd

But on the domain field, at what point do we expect a value in this field? After the user submits this form I assume? I think it makes sense before, so the user knows the action he's about to take

Isn't this a good case for the pre-validation mixin you created in the other PR? 😉

[agjohnson]

I was thinking about that, there might be a way. The tricky part is that if we send a prevalidation form error about google being an invalid option, that also affects the user selecting github SSO as an option.

There might be a general error up front for both though: "You need to connect a service account to use single sign-on"

[agjohnson]

I think it makes sense before, so the user knows the action he's about to take

True, this would be nicer UX. Is the domain field value already set before the user uses this form? I don't have Google SSO set up for testing yet so can't test it.

[stsewd]

True, this would be nicer UX. Is the domain field value already set before the user uses this form? I don't have Google SSO set up for testing yet so can't test it.

yeah, https://github.com/readthedocs/readthedocs-corporate/blob/a1dc088a2ad5476110c5fac0f938c135e551653d/readthedocsinc/acl/sso/forms.py#L62-L62

[agjohnson]

I tried to improve this all with

• https://github.com/readthedocs/readthedocs-corporate/pull/1741

I like the end result and the extra validation errors. Choice field is definitely the way to go I feel, it was hard to get a nice UX as a disabled or read-only text field.