Skip to content
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.

Sign up for GitHub

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

SSH: add reattach key action #553

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

SSH: add reattach key action #553

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92b0031
SSH: add reattach key action
stsewd Jan 7, 2025
0114c95
Use message
stsewd Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 46 additions & 17 deletions readthedocsext/theme/templates/projects/partials/edit/keys_list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,57 @@
{% extends "includes/crud/table_list.html" %}

{% load i18n %}

{% load core_tags %}
{% load privacy_tags %}
{% load projects_tags %}
{% load trans blocktrans from i18n %}

{% block top_left_menu_items %}
{% endblock top_left_menu_items %}

{% comment %}
Do not show an empty menu bar at the top. Once users can add SSH keys, we can
remove this override and enable the ``create_button`` block
{% endcomment %}
{% block top_menu %}
<div class="ui stackable text menu">
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is always for adding objects to a listing of objects, with a few exceptions for export/download buttons. I would not repurpose this to fit this UI or other one-off buttons and would move this UI outside this partial template and put the content in a .ui.segment.

This also fits in the listing as a small button menu for each key, other UIs use this approach and it would fit if we make SSH keys many to many

Copy link
Member

Choose a reason for hiding this comment

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

It would be easier to do this the first time if we had documented patterns or guidelines for this stuff. Whenever I edit the templates it feels like I'm just guessing at where to put stuff and what classes to use, and then just waiting to be told I'm doing it wrong. There has to be a better pattern.

<div class="right menu">
<div class="item">
<form class="ui form"
method="post"
action="{% url "projects_keys_reattach" project_slug=project.slug %}">
{% csrf_token %}
<button class="ui button"
{% if not can_reattach_key_automatically %}disabled{% endif %}>
<i class="fa-duotone fa-refresh icon"></i>
{% trans "Reattach to Git provider" %}
</button>
</form>
</div>
</div>
</div>

{% if not can_reattach_key_automatically %}
<div class="ui warning message">
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like an error state the user needs to fix. We don't want to send the user down the rabbit hole of fixing the connection to a repository unless their builds aren't working though.

The user isn't going to know of what the difference is between reattaching and "can't automatically reattach", this is fairly technical.

I think this content should be reframed a bit. If the user can reattach an SSH key, the message around this button should be "If you are having trouble building, you can reattach the SSH key". This is missing from the explanation right now.

In the case the user can't reattach, I don't think it should be an error about not being able to reattach, but instead "To enable SSH key management, connect a repository". That is, avoid describing this visually and with copy as an error state.

All of this might fit better in a modal really, but either options works for a first pass.

<div class="header">
{% blocktrans trimmed %}
SSH key can't be automatically reattached to Git provider
{% endblocktrans %}
</div>
<p>
{% if not project.remote_repository %}
{% url "projects_edit" project_slug=project.slug as project_edit_url %}
{% blocktrans trimmed with project_edit_url=project_edit_url %}
You need to <a href="{{ project_edit_url }}">connect this project to a Git repository</a> first.
{% endblocktrans %}
</p>
{% else %}
{% url "socialaccount_connections" as socialaccount_connections_url %}
{% blocktrans trimmed with socialaccount_connections_url=socialaccount_connections_url %}
You need to <a href="{{ socialaccount_connections_url }}">connect your account to a Git provider associated with this project</a> first.
{% endblocktrans %}
<p>
{% endif %}
{% blocktrans trimmed %}
Alternatively, you can
<a href="https://docs.readthedocs.io/en/stable/guides/importing-private-repositories.html#copy-your-project-s-public-key">manually add the SSH key to your Git provider</a>.
{% endblocktrans %}
Comment on lines +47 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not give two separate actions in this UI, I'd drop this suggestion. It can be a help topic if we need but the first UI element shouldn't point users to manually manage this.

</p>
</div>
{% endif %}
{% endblock top_menu %}
{% block create_button %}
{% comment %}
{# TODO this is not supported currently, and is an admin only action #}
{% url "..." project.slug as create_url %}
{% trans "Add SSH key" as create_text %}
{% include "includes/crud/create_button.html" with url=create_url text=create_text %}
{% endcomment %}
{% endblock create_button %}

{% block list_placeholder_icon_class %}
fa-duotone fa-lock-keyhole
Expand Down