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.

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

Show APIv3 Token under Profile settings #5954

Merged
merged 6 commits into from Jul 23, 2019
Merged

Show APIv3 Token under Profile settings #5954

merged 6 commits into from Jul 23, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jul 18, 2019

Some comments here:

  • Currently we support one Token per User (it's a OneToOne relation on the model that comes with DRF) --although, the Django view it's a ListView
  • I added this token under the user's settings since it's where I think it makes the most sense
  • Once we support Scoped Tokens, we can refactor this page to show them all here mentioning their limitations/allowed permissions.
  • In case the Scoped Tokens would be Project-based, we can add a similar page under Project's Admin.

I'm not sold on this page and I accept comments and probably a complete re-work or re-structure (*)

Screenshot_2019-07-18_14-16-44

(*) my other idea was to just add a message like: "Hey there! We are testing our APIv3 and you were given early access to help us with your feedback. Here you have the token to start testing it out: {token}." in a gray small box similar to what we shown under Project Admin Settings.

Example,

Screenshot_2019-07-18_14-27-34

@humitos humitos force-pushed the humitos/api-token-admin branch from 291b52c to 63ddcb7 Jul 18, 2019
Copy link
Member

@stsewd stsewd left a comment

I think tokens should be treated like we do with env variables, only see them in the creation step (github does this). We could allow users token creation under a feature flag instead of us doing it manually.


def get_queryset(self):
# Token has a OneToOneField relation with User
return Token.objects.filter(user__in=[self.request.user])
Copy link
Member

@stsewd stsewd Jul 18, 2019

Why can't this be just Token.objects.filter(user=self.request.user)?


class TokenMixin:

"""Environment Variables to be added when building the Project."""
Copy link
Member

@stsewd stsewd Jul 18, 2019

This docstring doesn't look correct

@humitos
Copy link
Member Author

@humitos humitos commented Jul 18, 2019

I think tokens should be treated like we do with env variables, only see them in the creation step (github does this).

I agree this is the best pattern, although we don't allow users to create these tokens right now.

We could allow users token creation under a feature flag instead of us doing it manually.

How we can use feature flags based on users and not on projects?

Copy link
Member

@ericholscher ericholscher left a comment

This looks fine for now, but we need more messaging for users so they aren't confused about why they can't set one up.

@@ -25,3 +25,13 @@
name='account_advertising',
),
]

tokens_urls = [
Copy link
Member

@ericholscher ericholscher Jul 19, 2019

Why did we add a new list here instead of just putting it in the urlpatterns? Could use a comment if there's a reason.

Copy link
Member Author

@humitos humitos Jul 22, 2019

I just followed the pattern that we have in other urls.py file.

Although, I think I need to use a different name for the first list and add it to urlpatterns later. This way allow us to decide which URLs to import and define from corporate. Actually, we don't want to publish this new URL in Corporate since there is not going to be API Tokens yet.


{% block edit_content_header %} {% trans "Personal Access Tokens" %} {% endblock %}

{% block edit_content %}
Copy link
Member

@ericholscher ericholscher Jul 19, 2019

We definitely need something saying that users can't set them up yet, and to contact us to create one.

Copy link
Member Author

@humitos humitos Jul 22, 2019

I added a message. Let me know.

Screenshot_2019-07-22_13-55-38

@@ -49,6 +49,7 @@ <h2>
<li class="{% block profile-admin-social-accounts %}{% endblock %}"><a href="{% url 'socialaccount_connections' %}">{% trans "Connected Services" %}</a></li>
<li class="{% block profile-admin-change-password %}{% endblock %}"><a href="{% url 'account_change_password' %}">{% trans "Change Password" %}</a></li>
<li class="{% block profile-admin-change-email %}{% endblock %}"><a href="{% url 'account_email' %}">{% trans "Change Email" %}</a></li>
<li class="{% block profile-admin-tokens %}{% endblock %}"><a href="{% url 'profiles_tokens' %}">{% trans "Personal Access Tokens" %}</a></li>
Copy link
Contributor

@davidfischer davidfischer Jul 19, 2019

Is there a reason this is "Personal Access Tokens". I think "API Tokens" seems a bit clearer.

Copy link
Member Author

@humitos humitos Jul 20, 2019

No. I wasn't sure how to name it and I took the name from GitHub.

Copy link
Member

@ericholscher ericholscher Jul 22, 2019

👍 on API tokens.

readthedocs/templates/profiles/private/token_list.html Outdated Show resolved Hide resolved
readthedocs/templates/profiles/private/token_list.html Outdated Show resolved Hide resolved
readthedocs/templates/profiles/private/token_list.html Outdated Show resolved Hide resolved
@@ -49,6 +49,7 @@ <h2>
<li class="{% block profile-admin-social-accounts %}{% endblock %}"><a href="{% url 'socialaccount_connections' %}">{% trans "Connected Services" %}</a></li>
<li class="{% block profile-admin-change-password %}{% endblock %}"><a href="{% url 'account_change_password' %}">{% trans "Change Password" %}</a></li>
<li class="{% block profile-admin-change-email %}{% endblock %}"><a href="{% url 'account_email' %}">{% trans "Change Email" %}</a></li>
<li class="{% block profile-admin-tokens %}{% endblock %}"><a href="{% url 'profiles_tokens' %}">{% trans "Personal Access Tokens" %}</a></li>
Copy link
Member

@ericholscher ericholscher Jul 22, 2019

👍 on API tokens.

Copy link
Member

@ericholscher ericholscher left a comment

I'm 👍 shipping w/ the small changes I mentioned above.

humitos and others added 3 commits Jul 23, 2019
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@humitos
Copy link
Member Author

@humitos humitos commented Jul 23, 2019

Merging once the tests pass.

@humitos humitos merged commit 769a228 into master Jul 23, 2019
2 checks passed
@humitos humitos deleted the humitos/api-token-admin branch Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants