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

build_url added to all API v3 build endpoints #7373

Merged
merged 7 commits into from Aug 31, 2020

Conversation

saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Aug 9, 2020

closes #7361

@saadmk11 saadmk11 requested a review from a team Aug 9, 2020
Copy link
Member

@stsewd stsewd left a comment

Thanks, this should be grouped under an ulrs key, similar to

urls = ProjectURLsSerializer(source='*')

@saadmk11 saadmk11 requested a review from stsewd Aug 11, 2020
"project": "https://readthedocs.org/projects/project/",
"version": "https://readthedocs.org/dashboard/project/version/v1.0/"
Copy link
Member

@stsewd stsewd Aug 12, 2020

Choose a reason for hiding this comment

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

The project and version URL are going to be the same on every element of the list, not sure about that (we do the same in _links already). I think maybe just add the build url for now, and see if we need the others later.

Copy link
Member Author

@saadmk11 saadmk11 Aug 13, 2020

Choose a reason for hiding this comment

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

Okay sure! updated :)

@stsewd stsewd requested a review from agjohnson Aug 12, 2020
@saadmk11
Copy link
Member Author

saadmk11 commented Aug 13, 2020

I think this #7296 is related

stsewd
stsewd approved these changes Aug 13, 2020
Copy link
Member

@humitos humitos left a comment

Made a suggestion to match the same structure than _links.

"urls": {
"build": "https://readthedocs.org/projects/project/builds/1/"
},
Copy link
Member

@humitos humitos Aug 25, 2020

Choose a reason for hiding this comment

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

What about showing the same URLs we are listing in _links?

  • _self
  • project
  • version

I think it makes sense to keep that matching between the two. _links refers to API URLs and urls refer to user/dashboard URLs. Both with the same naming.

Copy link
Member Author

@saadmk11 saadmk11 Aug 26, 2020

Choose a reason for hiding this comment

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

@humitos Yeah! That makes sense 👍 . Updated the PR

@saadmk11 saadmk11 requested a review from humitos Aug 26, 2020
@@ -88,6 +88,31 @@ def get_project(self, obj):
return self._absolute_url(path)


class BuildURLsSerializer(BaseLinksSerializer, serializers.Serializer):
_self = serializers.URLField(source='get_full_url')
Copy link
Member

@stsewd stsewd Aug 26, 2020

Choose a reason for hiding this comment

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

we are not using self in the rest of url serializers, I think we can leave it as build

Copy link
Member Author

@saadmk11 saadmk11 Aug 26, 2020

Choose a reason for hiding this comment

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

sure!

Copy link
Member

@humitos humitos left a comment

Thanks for your contribution! 🎉

@humitos humitos merged commit 0da9715 into readthedocs:master Aug 31, 2020
2 checks passed
@saadmk11 saadmk11 deleted the apiv3-build-url branch Aug 31, 2020
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.

API v3 version build endpoint doesn't return build URL
3 participants