Skip to content

Conversation

@auraz
Copy link
Contributor

@auraz auraz commented Nov 21, 2013

When ID was removed from unit html (in https://github.com/edx/edx-platform/pull/1680) , transcripts were left broken, as they use id (location) and get_item(item_location) to obtain transcripts files.

This is CAT-1 blades bug: https://edx-wiki.atlassian.net/browse/BLD-530 (Can't import or add transcripts for video)

In longer term - @cahrens what should be changed in transcripts files to support (https://github.com/edx/edx-platform/pull/1680) change?

@polesye can we add tests for this case?

@Lyla FYI

@ghost ghost assigned cahrens Nov 21, 2013
@cahrens
Copy link

cahrens commented Nov 21, 2013

Sorry about that-- I didn't realize that was how the ID was being obtained for transcripts. We need to change the transcripts python code to expect locators instead of locations. We actually have a story for that (STUD-870), which we should be working on soon.

@cahrens
Copy link

cahrens commented Nov 21, 2013

Is there a more explicit test that you can add that verifies the transcripts are working properly?

@auraz
Copy link
Contributor Author

auraz commented Nov 21, 2013

@cahrens Thank you. Great, that you have story for that.

@polesye
Copy link
Contributor

polesye commented Nov 21, 2013

@cahrens
If ajax request in some reason fails (for example, 500 error) error message will be shown on frontend.
This additional assertion checks if some error message is shown after each ajax request.
Is this test okay for you?

@cahrens
Copy link

cahrens commented Nov 21, 2013

I guess that is OK if there is no other way to tell if uploading and modifying the transcripts actually worked. I was hoping for an integration test that uploaded or modified transcripts, then verified in the video instance that the transcripts are correct after the change. But perhaps that is not easy to test?

@polesye
Copy link
Contributor

polesye commented Nov 21, 2013

The last test in the transcripts.feature should cover situation described by you.

@cahrens
Copy link

cahrens commented Nov 21, 2013

But the existing test did not catch the bug I introduced. Didn't I completely break adding and updating transcripts?

@polesye
Copy link
Contributor

polesye commented Nov 21, 2013

Good point. I have updated this test.
It should catch the bug now.

@cahrens
Copy link

cahrens commented Nov 21, 2013

👍 after rebase.

polesye added a commit that referenced this pull request Nov 22, 2013
@polesye polesye merged commit 51fa7e0 into master Nov 22, 2013
@polesye polesye deleted the alex/fix-transcripts-500-error branch November 22, 2013 08:40
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.

4 participants