Skip to content

Conversation

@itsjeyd
Copy link
Member

@itsjeyd itsjeyd commented Sep 2, 2015

In the LMS, links of the form /jump_to_id/<unit identifier> are currently not translated/expanded correctly when they are part of a table column header. For example,

/jump_to_id/mentoring_reflecting_on_feedback

correctly expands to

https://openedx.gse.harvard.edu/courses/course-v1:HGSE+IOCEX+2015_T1/jump_to_id/mentoring_reflecting_on_feedback

when added to an HTML block. But when added to the column header of an Answer Recap table the expanded URL ends up missing information about the parent course (leading to an invalid link):

https://openedx.gse.harvard.edu/jump_to_id/mentoring_reflecting_on_feedback

This PR adds code that makes sure links to course units that are part of table column headers are expanded correctly inside the LMS.

Testing

  1. Download a copy of the course-v1:HGSE+IOCEX+2015_T1 course from the Harvard instance and import it into your local devstack.
  2. In the LMS, navigate to Courseware ⟶ Change Diary ⟶ Change Diary: Map and Fields and make sure you are viewing the second unit ("My Change Diary - Maps").
  3. Verify that the links in the table column headers can be used to successfully navigate to the corresponding units of the course.

@itsjeyd itsjeyd force-pushed the fix-invalid-links-in-tables branch from d4496e7 to b232a70 Compare September 2, 2015 16:57
@Kelketek
Copy link
Member

Kelketek commented Sep 2, 2015

👍 Works for me, code looks fine.

@antoviaque
Copy link
Member

Can we add a test for this?

@itsjeyd
Copy link
Member Author

itsjeyd commented Sep 3, 2015

@antoviaque The code conditionally uses functionality from the LMS runtime to translate jump_to_id URLs (as do other XBlocks). That functionality is not present in the workbench runtime, so the check that happens here will fail when the code runs outside of the LMS. So at this stage I'm not sure how we could go about implementing a test for the fix.

@antoviaque
Copy link
Member

@itsjeyd Could you mock the replace_jump_to_id_urls() method for the test, if it's not present in the workbench? You don't need to duplicate strictly the functionality, but if it could ensure the URL gets through this method when available, that might ensure we don't forget it again in future refactorings.

@itsjeyd
Copy link
Member Author

itsjeyd commented Sep 3, 2015

@antoviaque Thanks for the suggestions, mocking replace_jump_to_id_urls worked. I extended the integration test for MentoringTableBlock to cover this fix.

@Kelketek Please have another look.

@Kelketek
Copy link
Member

Kelketek commented Sep 3, 2015

👍 if the tests pass. I've retriggered the failing build since it seems to be one of those random flaky ones.

itsjeyd added a commit that referenced this pull request Sep 3, 2015
Fix: Translate links in table column headers
@itsjeyd itsjeyd merged commit 9af13c3 into master Sep 3, 2015
@xitij2000 xitij2000 deleted the fix-invalid-links-in-tables branch December 13, 2019 06:42
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