Show location in the settings xblock. by oksana-slu · Pull Request #11999 · openedx/openedx-platform

oksana-slu · 2016-03-31T08:19:26Z

Background:
Review of xblock location is enabled. Field "display_block_location" is added to course settings; this is a field visibility of xblock location depends on. This functionality is an important part of Conditional Mode editing

Studio Updates:
The next file is modified: common/lib/xmodule/xmodule/course_module.py, common/lib/xmodule/xmodule/x_module.py.
The new tests are added common/test/acceptance/tests/studio/test_studio_container.py, common/lib/xmodule/xmodule/tests/test_course_module.py.
For new functionality, the next tests are corrected: common/lib/xmodule/xmodule/tests/test_editing_module.py, common/lib/xmodule/xmodule/tests/test_split_test_module.py, common/lib/xmodule/xmodule/tests/test_xml_module.py, common/test/acceptance/pages/studio/settings_advanced.py, lms/djangoapps/courseware/tests/test_video_mongo.py.

LMS Updates: None

openedx-webhooks · 2016-03-31T08:19:35Z

Thanks for the pull request, @oksana-slu! I've created OSPR-1217 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

nedbat · 2016-03-31T21:21:10Z

@oksana-slu I don't understand what this means. Maybe @scottrish has some context.

oksana-slu · 2016-04-01T10:14:55Z

@nedbat @scottrish
We have developed conditional mode which is enabled within Studio functionality. For using new conditional mode user needs to add location in source list (conditional setting, see picture ).

At the moment it is quite difficult to get this data in Open edX. So we add to advanced settings new field "Display block location". If user sets "true", every xblock will contain location (read only). This data can be added in settings of conditional mode.

marcotuts · 2016-04-19T02:17:38Z

This setting doesn't seem like something authors should need to set. What is the benefit of the conditional logic / flag to exposing the course ID / location? Could this be always included behind the scenes to an xblock? Is there any downside to doing this?

I ask because the expectation is that settings in the Settings tab for an XBlock should (whenever possible) be meaningful to course authors and not be configuration logic for xblock details.

jbarciauskas · 2016-05-11T17:27:20Z

@oksana-slu any thoughts based on @marcotuts 's feedback?

marcotuts · 2016-05-11T17:29:32Z

edit: sorry I got my PRs confused. I've updated my comment here:

I now understand that the location is read only and added here as metadata to be copied in for the conditional block.

My feedback summary is this:
1.) Could this be rendered at the bottom of the settings list, and given that this is metadata / details and not a setting, could this be rendered as text instead of as an input field that is disabled?

Thanks!

nedbat · 2016-06-09T20:55:42Z

@oksana-slu are you still interested in pursuing this?

oksana-slu · 2016-06-10T07:09:41Z

@nedbat
Hello!
Sorry for delaying.(I miss your comment). We are still interested in this PR.
It could be rendered as a button, but we need time to change the business logic of location feature.
If we return to this in month, will it be ok?
Does this PR influence on https://github.com/edx/edx-platform/pull/11710?
thank you

oksana-slu · 2016-06-16T14:16:48Z

@marcotuts @nedbat
Could I ask a question.
I adapted source code according the requirements.
But taking into account your comments, my previous commit is not of current interest. Should I make new PR or just change the source code in current PR.
Thanks

jbarciauskas · 2016-06-16T17:11:36Z

@oksana-slu it's up to you - if the historical context is useful, then we should continue on this PR. If it's more helpful to restart the discussion, then close and create a new one

nedbat · 2016-06-23T18:41:30Z

@oksana-slu Feel free to ping people here in a comment when you are ready for more review.

oksana-slu · 2016-06-24T07:56:14Z

@nedbat
I have updated PR with your comments, review it please.

cahrens · 2016-06-28T18:40:25Z

Note that tests are not running on this PR, @nedbat

benpatterson · 2016-06-28T18:40:27Z

Can one of the admins verify this patch?

cahrens · 2016-06-28T18:59:28Z

    <% }) %>
+    <li class="field comp-setting-entry metadata_entry">
+        <div class="wrapper-comp-setting">
+            <label class="label setting-label"><%= gettext("Location id") %></label>


Should be using <%- instead of <%= (both this line and the next) to ensure the text is escaped.

jbarciauskas · 2016-06-28T20:03:07Z

Jenkins ok to test

jbarciauskas · 2016-06-28T20:03:23Z

Jenkins add to whitelist

oksana-slu · 2016-06-29T07:24:45Z

@cahrens
I fixed everything what you asked.
Thanks

jbarciauskas · 2016-06-29T12:04:31Z

jenkins run all

nedbat · 2016-06-29T12:10:18Z

Jenkins ok to test

jbarciauskas · 2016-06-29T13:03:41Z

jenkins run all

jbarciauskas · 2016-06-29T13:29:24Z

@oksana-slu looks like there's a build error - can you look into it?

cahrens · 2016-06-29T13:39:31Z

FYI that the safe templating change was just something I noticed while looking over OSPRs-- I'm not actually reviewing this PR. We still need product sign-off on this feature.

andy-armstrong · 2016-08-31T19:04:38Z

jenkins run all

andy-armstrong · 2016-08-31T19:06:19Z

@oksana-slu I'm going to review your PR this week. Can you fix the rebase issue (currently, GitHub sees >250 commits!) and let me know when there is a sandbox available. Thanks!

oksana-slu · 2016-09-05T12:00:17Z

@andy-armstrong
I fixed the rebase issue.
We updated the sandbox with location_id
LMS: https://edx-show-location-id.raccoongang.com
Studio: https://studio-show-location-id.raccoongang.com

One test is failed, I will fix it asap
Thanks

andy-armstrong · 2016-09-06T13:43:58Z

jenkins run bokchoy

andy-armstrong · 2016-09-06T13:49:41Z

@oksana-slu this looks good to me. 👍

@marcotuts the changes appear to have addressed your feedback about the rendering of the ID. If you want to give thumbs, then I'll go ahead and merge this.

catong · 2016-09-06T13:57:19Z

@andy-armstrong Did "id" in the string get changed to "ID"? 👍 from me if that is done.

catong · 2016-09-06T13:58:07Z

Ah, confirmed that it has been updated. Thanks!

marcotuts · 2016-09-06T16:39:45Z

Thanks for the updated FYI @andy-armstrong.

👍 from me!

andy-armstrong · 2016-09-12T19:08:18Z

@oksana-slu will you have time to fix the Bok Choy test failure this week? It would be great to have this PR merged. Thanks!

oksana-slu · 2016-09-13T06:21:03Z

@andy-armstrong
This test "test_create_book_message" was ok locally.
http://prntscr.com/che9zo

Could you please explain what should I do in such situation?
thanks

andy-armstrong · 2016-09-13T14:25:27Z

@oksana-slu That's very frustrating, and I don't have much advice to offer. One useful thing is to find the screenshot that is taken when the test fails. In this case it is here:

https://build.testeng.edx.org/job/edx-platform-bok-choy-pr/25747/artifact/test_root/log/shard_2/common.test.acceptance.tests.studio.test_studio_textbooks.TextbooksTest.test_create_first_book_message.png

The test is looking for a message, and the screenshot seems to be showing the correct one, so that doesn't help. Maybe there's a timing problem with the test that isn't introduced by your changes.

Given that your changes seem unrelated, I suggest rebasing again and see if maybe that fixes it. If it doesn't, then I suggest introducing a wait into the test so that it waits for the element to be available.

andy-armstrong · 2016-09-13T14:25:34Z

jenkins run bokchoy

catong · 2016-09-13T18:22:26Z

@oksana-slu Sorry for this late change. After consulting with @marcotuts we realize we need to change the name of this ID to avoid confusion with the "Location ID" that is visible in the Studio course outline sidebar.
Can you please change "Location ID" to "Component Location ID"?
Thanks, please let us know if you have any questions. FYI @andy-armstrong

shaunagm · 2016-09-15T17:38:00Z

@andy-armstrong @catong @oksana-slu The tests are passing and the ID name change has been made. Is this ready to be merged?

catong · 2016-09-15T17:45:44Z

👍

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage product review PR requires product review before merging and removed needs triage labels Mar 31, 2016

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed product review PR requires product review before merging labels May 11, 2016

oksana-slu mentioned this pull request Jun 15, 2016

Add ui in studio for conditional_module. #11710

Merged

cahrens reviewed Jun 28, 2016
View reviewed changes

openedx-webhooks added engineering review and removed awaiting prioritization labels Aug 31, 2016

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels Aug 31, 2016

Show location in the settings xblock.

943757a

oksana-slu force-pushed the kssl/show_location_id_xblock branch from 0c02e12 to 943757a Compare September 4, 2016 12:53

openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 6, 2016

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels Sep 7, 2016

Changed "Location ID" to "Component Location ID"

e3f55af

andy-armstrong merged commit 8b8339b into openedx:master Sep 16, 2016

dcadams mentioned this pull request Sep 16, 2016

Display unobtrusive component locations in studio Stanford-Online/edx-platform#511

Merged

oksana-slu deleted the kssl/show_location_id_xblock branch July 12, 2018 06:40

Masergik mentioned this pull request Sep 4, 2019

[CXB-5] Add component location ID in studio edit raccoongang/xblock-video#368

Open

Uh oh!

Conversation

oksana-slu commented Mar 31, 2016

Uh oh!

openedx-webhooks commented Mar 31, 2016

Uh oh!

nedbat commented Mar 31, 2016

Uh oh!

oksana-slu commented Apr 1, 2016

Uh oh!

marcotuts commented Apr 19, 2016

Uh oh!

jbarciauskas commented May 11, 2016

Uh oh!

marcotuts commented May 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedbat commented Jun 9, 2016

Uh oh!

oksana-slu commented Jun 10, 2016

Uh oh!

oksana-slu commented Jun 16, 2016

Uh oh!

jbarciauskas commented Jun 16, 2016

Uh oh!

nedbat commented Jun 23, 2016

Uh oh!

oksana-slu commented Jun 24, 2016

Uh oh!

cahrens commented Jun 28, 2016

Uh oh!

benpatterson commented Jun 28, 2016

Uh oh!

cahrens Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

jbarciauskas commented Jun 28, 2016

Uh oh!

jbarciauskas commented Jun 28, 2016

Uh oh!

oksana-slu commented Jun 29, 2016

Uh oh!

jbarciauskas commented Jun 29, 2016

Uh oh!

nedbat commented Jun 29, 2016

Uh oh!

jbarciauskas commented Jun 29, 2016

Uh oh!

jbarciauskas commented Jun 29, 2016

Uh oh!

cahrens commented Jun 29, 2016

Uh oh!

andy-armstrong commented Aug 31, 2016

Uh oh!

andy-armstrong commented Aug 31, 2016

Uh oh!

oksana-slu commented Sep 5, 2016

Uh oh!

andy-armstrong commented Sep 6, 2016

Uh oh!

andy-armstrong commented Sep 6, 2016

Uh oh!

catong commented Sep 6, 2016

Uh oh!

catong commented Sep 6, 2016

Uh oh!

marcotuts commented Sep 6, 2016

Uh oh!

andy-armstrong commented Sep 12, 2016

Uh oh!

oksana-slu commented Sep 13, 2016

Uh oh!

andy-armstrong commented Sep 13, 2016

Uh oh!

andy-armstrong commented Sep 13, 2016

Uh oh!

catong commented Sep 13, 2016

Uh oh!

shaunagm commented Sep 15, 2016

Uh oh!

marcotuts commented May 11, 2016 •

edited

Loading