-
Notifications
You must be signed in to change notification settings - Fork 0
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
SAML: port template #348
base: main
Are you sure you want to change the base?
SAML: port template #348
Conversation
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
|
||
<div class="ui padded segment"> | ||
<h4> | ||
{% trans "SAML integration details" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like these sections are swapped. These feel like "settings" instead of details. The read only attributes above, labeled "settings" feel like they should be labeled "integration details".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They both are settings, ones are used to create the integration from the identify provider, and the others are the values we are using to connect to that integration, plus the domain we are linking to that integration. But, yeah, not really sure how to name each section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read only attributes that users use on their own SAML provider are not settings to our users, the user cannot set them. I would try to avoid ambiguity here and be more descriptive with what these values actually are or do for the user.
- For the top section: "Details for your SAML provider" or "Configure your SAML provider"
- For the form section where the user does enter settings, all other forms in admin views use "Add ..." and "Update ..." as titles -- so "Add SAML integration" and "Update SAML integration" based on whether or not the form is bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't allowing updating the integration yet https://github.com/readthedocs/readthedocs-corporate/issues/1781.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted on that issue, we already allow full control of other auth mechanisms, so it seems worth keeping these consistent instead of special casing SAML auth. We should improve the UX around auth changes in the future more broadly, but SAML is not particularly unique here.
<div class="field"> | ||
<label>{% trans "Domain" %}</label> | ||
<div class="ui fluid input"> | ||
<input type="text" value="{{ domain }}" readonly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not a form
instance that we can use here instead of making the form manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we fetch all the attributes from a single URL, we don't have a form for all attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to do a bit more with the form here. This UI is making new patterns where I'd like to instead keep things consistent across all the views.
It's okay to show the read only values as a supplemental to a form instance, but the form should always be treated as a form at very least. That is, the two user editable fields (domain, metadata URL) should always be part of a form display through Crispy. Right now, after submitting, there doesn't seem to be a way to change or delete these fields.
With those fields displaying as a form, I would then display the readonly attributes separately, as to not confuse them with the user editable fields (domain and metadata URL).
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
readthedocsext/theme/templates/organizations/settings/saml.html
Outdated
Show resolved
Hide resolved
|
||
<div class="ui padded segment"> | ||
<h4> | ||
{% trans "SAML integration details" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read only attributes that users use on their own SAML provider are not settings to our users, the user cannot set them. I would try to avoid ambiguity here and be more descriptive with what these values actually are or do for the user.
- For the top section: "Details for your SAML provider" or "Configure your SAML provider"
- For the form section where the user does enter settings, all other forms in admin views use "Add ..." and "Update ..." as titles -- so "Add SAML integration" and "Update SAML integration" based on whether or not the form is bound.
<div class="field"> | ||
<label>{% trans "Domain" %}</label> | ||
<div class="ui fluid input"> | ||
<input type="text" value="{{ domain }}" readonly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to do a bit more with the form here. This UI is making new patterns where I'd like to instead keep things consistent across all the views.
It's okay to show the read only values as a supplemental to a form instance, but the form should always be treated as a form at very least. That is, the two user editable fields (domain, metadata URL) should always be part of a form display through Crispy. Right now, after submitting, there doesn't seem to be a way to change or delete these fields.
With those fields displaying as a form, I would then display the readonly attributes separately, as to not confuse them with the user editable fields (domain and metadata URL).
</div> | ||
</div> | ||
|
||
<div class="ui padded segment"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other views do no use segments to wrap the forms, the segment here can be removed so the form shows with no border.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have the problem from #348 (comment). |
This is just a basic form to show information about the integration.
Ref https://github.com/readthedocs/readthedocs-corporate/pull/1770