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

Store version media availability #6278

Merged
merged 2 commits into from Oct 14, 2019

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 10, 2019

This PR allows the availability of particular download media to be saved on the version at the end of the build process. For example, does a particular version have a PDF? ePub? etc.

Currently, we rely on the presence of a file existing at a certain filesystem path to determine whether media/downloads are available. As we move toward storage being an abstraction on top of the filesystem where it is possibly remote (eg. Cloud storage) we don't want to have to query remote storage to know whether a version has a PDF.

This PR is the first step in changing that. The next is to write a job which calculates has_pdf/has_epub/has_htmlzip for the latest build of every version. Lastly, we can actually use the fields stored in the DB instead of checking the filesystem.

- Stored during the build process
- Does a particular version have a PDF? ePub? etc.
@davidfischer davidfischer requested a review from Oct 10, 2019
stsewd
stsewd approved these changes Oct 11, 2019
Copy link
Member

@stsewd stsewd left a comment

Makes sense to me 👍

Copy link
Member

@humitos humitos left a comment

Looks good to me.

@@ -90,6 +90,9 @@ class Meta:
'built',
'downloads',
'type',
'has_pdf',
'has_epub',
'has_htmlzip',
Copy link
Member

@humitos humitos Oct 14, 2019

Choose a reason for hiding this comment

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

shouldn't we add these to fields to APIv3 Version detail endpoint as well?

I'm not sure, since the download link will appear only if it exists. So, maybe it's redundant.

Copy link
Contributor

@agjohnson agjohnson Oct 14, 2019

Choose a reason for hiding this comment

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

These has_* fields feel like redundant fields for our API

Copy link
Contributor Author

@davidfischer davidfischer Oct 14, 2019

Choose a reason for hiding this comment

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

I agree it is a little awkward. These fields are used for writing by the build servers but not reading by the general public.

readthedocs/rtd_tests/tests/test_api.py Show resolved Hide resolved
Copy link
Member

@ericholscher ericholscher left a comment

Excited to have this working! 👍

Co-Authored-By: Manuel Kaufmann <humitos@gmail.com>
@agjohnson agjohnson merged commit cb56df4 into master Oct 14, 2019
3 checks passed
@agjohnson agjohnson deleted the davidfischer/store-version-media-availability branch Oct 14, 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

5 participants