search

openedx / xblock-image-explorer

GNU Affero General Public License v3.0
9 stars 39 forks source link

Kuldeep/MCKIN-7824 change static url description hotspots #48

Closed LanghaniKuldeep closed 6 years ago

LanghaniKuldeep commented 6 years ago

Replacing static URLs with absolute URLs for student_view_data in description and hotspots.

naeem91 commented 6 years ago

@pomegranited would you like to review or assign someone else from your team? I'm not sure who to assign that.

pomegranited commented 6 years ago

FYI @xitij2000 -- is this task on your radar?

xitij2000 commented 6 years ago

@pomegranited Nope will make a ticket for it now.

xitij2000 commented 6 years ago

@naeem91 You can ping me in such cases and I will take care of it.

naeem91 commented 6 years ago

sure @xitij2000 Thanks!

LanghaniKuldeep commented 6 years ago

@ahmedaljazzar Please have a look. I have made the requested changes.

iamjazzar commented 6 years ago

@LanghaniKuldeep, I left you a comment on the change. Please make sure to add some tests for the new functionality you're introducing..

LanghaniKuldeep commented 6 years ago

@ahmedaljazzar As in this I have created a private method and that is using another private method '_replace_static_from_url' and that is importing 'from static_replace import replace_static_urls' from edx-platform. Therefore I might not be able to write test this for description and hotspot.

iamjazzar commented 6 years ago

Can't you do at least some mockings?

LanghaniKuldeep commented 6 years ago

@ahmedaljazzar I guess not. That file is required from the platform or we have to setup mock and that would be a bit overkill for this scope.

mtyaka commented 6 years ago

@LanghaniKuldeep We are trying to improve test coverage of this XBlock by requiring all code changes to include tests for relevant parts of the code before approving he PR.

For this change I think it would be sufficient to add a test to existing unit test file, which verifies that all URLs included in the student_view_data response have been processed by the _replace_static_from_url function.

naeem91 commented 6 years ago

@ahmedaljazzar this is ready for another pass. Added some unit tests

iamjazzar commented 6 years ago

Thanks, @naeem91. Will start the review soon.

naeem91 commented 6 years ago

@ahmedaljazzar /@xitij2000 gentle review reminder!!

iamjazzar commented 6 years ago

This LGTM 👍

CC: @xitij2000, @mtyaka

xitij2000 commented 6 years ago

@naeem91 Could you please squash this? You can ping me and I can merge it once it's squashed.

naeem91 commented 6 years ago

@xitij2000 Commits squashed, good to merge now. Thanks!