SAML: port template

[stsewd]

This is just a basic form to show information about the integration.

Ref https://github.com/readthedocs/readthedocs-corporate/pull/1770

[stsewd]

Still have the problem from https://github.com/readthedocs/ext-theme/pull/348#discussion_r1610725000.

[agjohnson]

Its fine to start with this, but I'll note that I think we need to come back to this pattern of magic links -- for this and for SSO. I don't think the user is going to know the difference between the two links, and I'm sure if we look at the data users don't use these links. If we are telling owners to invite users, they should probably use the team invite UI.

[humitos]

If we are telling owners to invite users, they should probably use the team invite UI.

Agreed. I'd say that once an organization enables SAML, the team invite UI should expose this link somehow instead of being hidden under this specific UI. Ideally, there should be only one UI to invite members and that UI should point the users to the correct place depending on what authorization method they are using.

[stsewd]

@agjohnson I'm having a hard time figuring out what exactly is expected here... Something like this?

I really don't think we should allow changing the metadata URL yet or delete the integration without a better logic to handle "deactivation", since it's a very destructive change, it affects all users connected to that organization (and isn't easy to revert).

Also, this form isn't a model form, so fields are being manually initiated.

[agjohnson]

I really don't think we should allow changing the metadata URL yet or delete the integration without a better logic to handle "deactivation", since it's a very destructive change, it affects all users connected to that organization (and isn't easy to revert).

This is the new pattern I'm saying we skip. We already allow users to change, and potentially break, their project's authentication. This UI does not need to do anything different yet.

I noted this here, as well as a few places above:

• https://github.com/readthedocs/readthedocs-corporate/issues/1781#issuecomment-2125857203

Long term, I agree there is better UX we can add on these forms. But right now, all auth mechanisms, templates, and forms should use the same patterns, and currently the forms for other authentication changes warn the users as they are making changes but do not block the user from making these changes.

If your change is using a single {{ form | crispy }}, that is what all the other forms do and is correct. There should be no conditional read only rendering of a form that does not use {{ form | crispy }}. We can add warnings in later, that is not a change necessary now either.

Also, this form isn't a model form, so fields are being manually initiated.

Yup, that should be fine, as long as it is a Form instance, we should always use Crispy to display the form.

[stsewd]

If your change is using a single {{ form | crispy }}, that is what all the other forms do and is correct. There should be no conditional read only rendering of a form that does not use {{ form | crispy }}. We can add warnings in later, that is not a change necessary now either.

There are only two fields in that form (metadata URL and domain), the other fields are just extracted from the settings and aren't part of the Django form. Do you mean creating a form with those fields as read only and manually initiate them?

[agjohnson]

No, the read-only fields are okay for now as one-off, read only fields. We should eventually move this to application logic where we can though, this almost certainly doesn't need one-off template logic.

Currently, I am only talking about changing the two fields in the existing form. This form should always be displayed with {{ form | crispy }}, and should not use conditional logic to iterate over the form fields and manually display them.

[stsewd]

This form should always be displayed with {{ form | crispy }}, and should not use conditional logic to iterate over the form fields and manually display them.

I was already using form|crispy. I just added the update form, but it still has a conditional under to check if the integration is being created for the first time or not. And the other fields are manually displayed (since they aren't part of the form). Again, I'm not really sure if this is what you are expecting.

[agjohnson]

I was already using form|crispy

The issue was that the form was only displayed with crispy forms on creation. On update/detail view the form fields were manually generated. What you have here is close to what I was requesting.

We should track a follow up issue to fix the issues here:

• Display issues on spacing and positioning
• Headings are not updated
• Multiple {{ form | crispy }} -- all other views only display a single form so the rendered in predictable
• Drop magic links for proper team invites