Skip to content

Use singular transcript util to retrieve transcripts from contentstore - #17851

Closed
Qubad786 wants to merge 4 commits into
masterfrom
mrehan/test-util-on-prod
Closed

Use singular transcript util to retrieve transcripts from contentstore#17851
Qubad786 wants to merge 4 commits into
masterfrom
mrehan/test-util-on-prod

Conversation

@Qubad786

@Qubad786 Qubad786 commented Apr 2, 2018

Copy link
Copy Markdown
Contributor

@Qubad786

Qubad786 commented Apr 2, 2018

Copy link
Copy Markdown
Contributor Author

FYI @muhammad-ammar.

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests.

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests.

@Qubad786

Qubad786 commented Apr 4, 2018

Copy link
Copy Markdown
Contributor Author

@muhammad-ammar I am updating sandbox.

@Qubad786

Qubad786 commented Apr 4, 2018

Copy link
Copy Markdown
Contributor Author

This is ready for the review.

@Qubad786

Qubad786 commented Apr 4, 2018

Copy link
Copy Markdown
Contributor Author

jenkins run python

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests.

@muhammad-ammar muhammad-ammar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Qubad786 I have concerns over these changes. Please see me inline comments.

get_or_create_sjson,
generate_sjson_for_all_speeds,
get_video_transcript_content,
get_transcript_from_contentstore,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't we need to use get_transcript instead of get_transcript_from_contentstore?

@Qubad786 Qubad786 Apr 4, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, I am only aiming for contentstore part of the interface. So that we may find some further unseen scenario(i.e. not covered by our util).

response.content_type = transcript_mime_type
response.content_type = mime_type

elif dispatch.startswith('available_translations'):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes in this file are different from what we did in https://github.com/edx/edx-platform/pull/17718. Is this intentionally? Changes in the https://github.com/edx/edx-platform/pull/17718 are final and should be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@muhammad-ammar this is temporary just to make sure that our contentstore part of the interface works as smooth as it should be! :)

@Qubad786

Qubad786 commented Apr 4, 2018

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests.

@Qubad786 Qubad786 closed this Jul 10, 2018
@mushtaqak
mushtaqak deleted the mrehan/test-util-on-prod branch July 10, 2018 11:37
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.

3 participants