-
Notifications
You must be signed in to change notification settings - Fork 20
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
Added permission groups to user management page in console #2113
Conversation
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.
Looks good, thanks Michael. I added a few very minor comments inline!
physionet-django/console/templates/console/user_management.html
Outdated
Show resolved
Hide resolved
physionet-django/console/templates/console/user_management.html
Outdated
Show resolved
Hide resolved
physionet-django/console/templates/console/user_management.html
Outdated
Show resolved
Hide resolved
{% for group in groups %} | ||
<ul><li><a href="{% url 'user_group' group.name %}">{{ group.name }}</a></li></ul> | ||
{% empty %} | ||
<ul><li>No Groups</li></ul> |
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 slightly unnecessary to have a "No groups" bullet here. Could we instead say The user is a member of the following groups:...
if there is one or more groups and something like The user does not belong to a permission group.
if not?
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 now have:
<div class="col-md-9">
<ul>
{% for group in groups %}
<li><a href="{% url 'user_group' group.name %}">{{ group.name }}</a></li>
{% empty %}
The user does not belong to a permission group.
{% endfor %}
</ul>
</div>
For people who aren't in a group, this step will output The user does not belong to a permission group.
inside <ul>
tags which I think isn't quite right. I haven't tested, but what about something like this:
<br />
<h3>Permission groups</h3>
<hr>
<div class="row mb-1">
{% if groups %}
<div class="col-md-6">
The user is a member of the following groups:
</div>
<div class="col-md-9">
<ul>
{% for group in groups %}
<li><a href="{% url 'user_group' group.name %}">{{ group.name }}</a></li>
{% endfor %}
</ul>
</div>
{% else %}
<div class="col-md-6">
The user does not belong to a permission group.
</div>
{% endif %}
</div>
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 switched over to the new format, it made the else statement make more sense by not including the <ul>
tag
bdbe71f
to
52538a8
Compare
4d1e88e
to
a0ff2f5
Compare
thanks, looks good to me! |
This PR addresses #2105 by adding a permissions group section on the user's page in the console.
Currently, the template is set up to list "no group" if the user does not have any group affiliations, however, I'm open to suggestions on maybe hiding the section entirely if the user has no group affiliation.