Skip to content

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 12, 2025

Screenshot_2025-06-12_16-36-44

@humitos humitos requested a review from a team as a code owner June 12, 2025 14:37
@humitos humitos requested a review from agjohnson June 12, 2025 14:37
humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Jun 12, 2025
@stsewd
Copy link
Member

stsewd commented Jun 12, 2025

Do we want to list every maintainer? In the admin we just expose a link to the first maintainer, which makes sense, I don't think we ever had the need to impersonate a specific user, unless we need to debug an issue with that user, in that case, we can just do that from the admin.

@agjohnson
Copy link
Contributor

This whole modal is rendered publicly, because only the link is behind a if user.is_staff conditional. That is, if you open a public project page in a private session, you'll see the modal code.

The modal should be protected with a similar condition with this information in it.

@agjohnson
Copy link
Contributor

I don't think we ever had the need to impersonate a specific user

Yeah, I normally impersonate through the organization admin listing, so just the first owner. I could see a long list being not useful, but I also never use this debug menu anyways. If I'm jumping in to impersonate a user, it's through front or the admin dashboard.

@humitos
Copy link
Member Author

humitos commented Jun 16, 2025

I impersonate a specific user all the time. In particular to debug permissions issues when they can't add project or similar situations. Having this list will make this workflow a lot easier and quicker to follow.

If we are worried about listing too many owners/maintainers, we can limit the list to "the last 5 active users".

Also, I just wrapped the modal inside a is_staff conditional as suggested.

@agjohnson
Copy link
Contributor

I impersonate a specific user all the time.

Yeah pretty sure we all do this. We weren't saying we don't impersonate single users, we were saying when we debug organization or project issues we don't need to impersonate a specific (or multiple) maintainer/owner users.

For impersonating a single user or debugging a project, I use the impersonate links in Front via a support request.

For an organization, if the Front links aren't enough I impersonate through the links in the admin -- so just the first organization owner is enough.

I basically never use my own session to debug user issues though, so I don't use this menu either. So I don't mind what content goes into it. But generally, I would be hesitant to continue putting additional features in this staff user only UI. I'd rather these links and functionality end up in Front links or the admin so it's usable on business too.

@humitos humitos closed this Jun 17, 2025
@stsewd stsewd deleted the humitos/debug-impersonate branch June 17, 2025 14:35
@humitos humitos restored the humitos/debug-impersonate branch June 30, 2025 11:05
@humitos
Copy link
Member Author

humitos commented Jun 30, 2025

I want to re-open this and re-consider. It harms nobody and reduce support time.

I found myself again wanting to impersonate a user in the community site. The support request came via GitHub (so we don't have any of the quick links we have in Front).

While in the project page, I want to click on Debug and then click on the Impersonate link for debugging this. It saves a lot of time.

@humitos humitos reopened this Jun 30, 2025
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This seems fine for me. I don't think I'll remember to use it, but if it makes life better for folks it seems low risk 🤷 We should make sure it doesn't make the page slower tho -- but I think it won't render unless is_staff is true, which seems safe.

But generally, I would be hesitant to continue putting additional features in this staff user only UI. I'd rather these links and functionality end up in Front links or the admin so it's usable on business too.

I don't understand why this wouldn't work on both sites? I see the Debug menu on both I thought -- but agreed on making all the things easier.

@agjohnson
Copy link
Contributor

agjohnson commented Jun 30, 2025

I don't understand why this wouldn't work on both sites?

For projects that are set to private privacy level, I'm assuming staff users don't bypass those permission checks.

But like I said, I also never debug user issues through my session either, so I could be wrong on that. I go into the admin to impersonate the org/user in this case.

Reiterating from above, I don't care what goes into this menu but think @stsewd is correct on not listing every org owner/maintainer.

<h3>{% trans "Impersonate" %}</h3>
<div class="ui sub header">{% trans "Project maintainer" %}</div>
<div class="ui vertical list">
{% for user in project|admins %}
Copy link
Contributor

Choose a reason for hiding this comment

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

To @stsewd point, we really only need one here. If there is a case for more than one, it seems reasonable to slice down to several results. There are projects/orgs with dozens of users.

You could also just link to the admin view of the filtered list of all org owners or project owners instead of duplicating this information in the application.

Comment on lines +209 to +211
<div class="item">
<a href="{{ ADMIN_URL|cut:"/admin" }}/impersonate/{{ user.pk }}/" target="_blank">{{ user.username }}</a>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to nest these elements, links can be items too:

Suggested change
<div class="item">
<a href="{{ ADMIN_URL|cut:"/admin" }}/impersonate/{{ user.pk }}/" target="_blank">{{ user.username }}</a>
</div>
<a class="item" href="{{ ADMIN_URL|cut:"/admin" }}/impersonate/{{ user.pk }}/" target="_blank">{{ user.username }}</a>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants