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

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

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

This docstring doesn't look correct

@humitos
Copy link
Member Author

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

👍 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

Choose a reason for hiding this comment

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

👍 on API tokens.

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.

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

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

humitos commented Jul 23, 2019

Merging once the tests pass.

@humitos humitos merged commit 769a228 into master Jul 23, 2019
@humitos humitos deleted the humitos/api-token-admin branch July 23, 2019 10:51
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