-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
291b52c
to
63ddcb7
Compare
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 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]) |
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.
Why can't this be just Token.objects.filter(user=self.request.user)
?
|
||
class TokenMixin: | ||
|
||
"""Environment Variables to be added when building the Project.""" |
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.
This docstring doesn't look correct
I agree this is the best pattern, although we don't allow users to create these tokens right now.
How we can use feature flags based on users and not on projects? |
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.
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 = [ |
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.
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.
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 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 %} |
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 definitely need something saying that users can't set them up yet, and to contact us to create one.
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.
@@ -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> |
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.
Is there a reason this is "Personal Access Tokens". I think "API Tokens" seems a bit clearer.
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.
No. I wasn't sure how to name it and I took the name from GitHub.
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.
👍 on API tokens.
@@ -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> |
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.
👍 on API tokens.
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'm 👍 shipping w/ the small changes I mentioned above.
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>
….org into humitos/api-token-admin
Merging once the tests pass. |
Some comments here:
I'm not sold on this page and I accept comments and probably a complete re-work or re-structure (*)
(*) 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,