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

Ship API v3 #6169

Merged
merged 32 commits into from Oct 30, 2019
Merged

Ship API v3 #6169

merged 32 commits into from Oct 30, 2019

Conversation

humitos
Copy link
Member

@humitos humitos commented Sep 11, 2019

  • Remove message about contacting support to get early API v3 access
  • Remove all mentions to API v3 beta in the docs
  • Mark API v2 as deprecated
  • Add a note about where to get the access Token
  • Creates a Token each time a new User is created
  • Allow the user to Generate/Revoke the Token under their profile page
  • Standardize documentation (show only relevant fields when describing endpoints)
  • Remove half-baked docstrings from Browsable API
  • Remove not useful filters (Project, Version)
  • Leave documentation for relevant fields only

Screenshot_2019-09-11_23-42-41

Screenshot_2019-09-11_23-42-52

Before merging this PR (or deploy it), we need to run a Python command to create a Token per User.

* Remove message about contacting support to get early API v3 access
* Remove all mentions to API v3 beta in the docs
* Mark API v2 as deprecated
* Add a note about where to get the access Token
@humitos humitos requested a review from Sep 11, 2019
@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Sep 11, 2019

Before merging this PR (or deploy it), we need to run a Python command to create a Token per User.

I gave this PR a short try and there was no way for users to create or revoke access tokens. That was fine for beta but I think for general use users need to be able to create and revoke them. If users can create their own access tokens, we don't need to create them for users and we can probably remove the signal too.

@humitos
Copy link
Member Author

@humitos humitos commented Sep 11, 2019

@davidfischer makes sense. I will make the changes to allow Create/Delete from the profile settings. Thanks!

@humitos
Copy link
Member Author

@humitos humitos commented Sep 11, 2019

@davidfischer I updated the PR with the ability to Generate/Revoke API Tokens under the profile page. Currently, it allows the user to have only one Token max. Let me know if you think this is the path to go forward.

Eventually, when we implement scope-based tokens, we will allow the user to manage more than one token at the same time.

stsewd
stsewd approved these changes Sep 12, 2019
Copy link
Member

@stsewd stsewd left a comment

LGTM

readthedocs/profiles/views.py Show resolved Hide resolved
readthedocs/profiles/views.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

@humitos humitos commented Sep 12, 2019

We may decide what to return in subprojects endpoint before shipping this as "stable": #6157

I'm blocking this for now so we can think about the proper approach first.

@humitos humitos added the Status: blocked label Sep 12, 2019
humitos and others added 2 commits Sep 12, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Sep 30, 2019

Before shipping it, I'd like to do a second pass review to all the code (we may want to remove some non-useful filters, for example) and documentation (maybe a refactor or cleaning).

@humitos humitos requested a review from Oct 1, 2019
@humitos humitos removed the Status: blocked label Oct 8, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Oct 8, 2019

Removing "Status: blocked" label and calling for a last review of @readthedocs/core team before merging. I'd like to have this deployed in the next deploy hopefully.

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 8, 2019

@humitos we should resolve what to do around privacy levels first

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 8, 2019

The docs for redirects don't mention what kind of redirects can be created, and also, not all redirects require a from or to urls

stsewd added a commit to stsewd/readthedocs.org that referenced this issue Oct 8, 2019
Some fields are not used.
I'm brining some changes from readthedocs#6169
to avoid merge conflicts.
@humitos
Copy link
Member Author

@humitos humitos commented Oct 14, 2019

Blocked on #6275 for now. We should merge that one first.

@humitos humitos added Status: blocked and removed Status: blocked labels Oct 14, 2019
Copy link
Member

@stsewd stsewd left a comment

Some examples from the docs are still showing privacy_level in the response.

docs/api/v3.rst Outdated Show resolved Hide resolved
docs/api/v3.rst Outdated Show resolved Hide resolved
docs/api/v3.rst Outdated Show resolved Hide resolved
docs/api/v3.rst Outdated Show resolved Hide resolved
docs/api/v3.rst Outdated Show resolved Hide resolved
Co-Authored-By: Santos Gallegos <santos_g@outlook.com>
@humitos
Copy link
Member Author

@humitos humitos commented Oct 22, 2019

Some examples from the docs are still showing privacy_level in the response.

Can you point me to them? I'm not finding anyone.

humitos added 3 commits Oct 22, 2019
It was removed by mistake. Probably while solving a merge conflict
@humitos humitos requested a review from Oct 22, 2019
@humitos humitos mentioned this pull request Oct 22, 2019
4 tasks
@stsewd
Copy link
Member

@stsewd stsewd commented Oct 22, 2019

Can you point me to them? I'm not finding anyone.

Can't see them anymore, guess merging master was missing?

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 22, 2019

Looks like the failing tests are not related to the esling thing, btw

stsewd
stsewd approved these changes Oct 28, 2019
@stsewd stsewd requested a review from Oct 28, 2019
@humitos humitos merged commit 4dc1aa0 into master Oct 30, 2019
3 checks passed
@humitos humitos deleted the humitos/ship-api-v3 branch Oct 30, 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants