Prevent overlapping drop placements #92

e-kolpakov · 2016-08-09T15:35:00Z

Description: This PR updates item placement logic:

Zone alignment "none" (aka no alignment) is removed, dropped items always use the "aligned" layout option rather than being placed where the learner dropped them.
"Max items per zone" setting added to allow limiting max number of items dropped into a single zone - course authors can use this setting to provide some kind of a hint, or to prevent visual overflow when too many items are dropped into single zone.

JIRA: SOL-1979

Discussions: SOL-1979 and #83 (comment)

Sandboxes:

Note: at the moment #86 was merged, but #87 was not, and this PR does not include changes from #87; as a result, DnDv2 on sandbox show "Submit" button, but it does not do anything - this is an expected behavior until #87 is merged

Testing instructions:

Items alignment:

Open new or existing DnDv2 block in studio, open editor, go to "Zones" tab. Expected: zone "Alignment" dropdown does not have "none" option; default is "center"
Open DnDv2 XBlock in LMS.
Make sure that items dropped into a zone are aligned according to zone alignment configuration.
Existing zones with "none" alignment should be treated as having "center"alignment; this should be the case even if an XBlock was not updated in Studio after this PR is applied.

Max items per zone:

Open new or existing DnDv2 block in studio, open editor, go to "Items" tab. Expected: "Max items per zone" is displayed.
Setting "max items per zone" to ~~zero or~~ empty string has no further effect (make sure you can put as much items into a single dropzone as you wish in LMS)
Setting "max items per zone" to zero or negative value ~~should cause validation errors~~ will result in value being set to empty string
Setting "max items per zone" to positive value will validate item placement. If any zone has more "correct" items than "max items per zone" a validation error will be shown, and XBlock will not be updated.
After setting "max items per zone" to positive value ~~and passing the validation~~, publish the unit and open it in LMS. Expected: can only put up to "max items per zone" items into a single zone.
Note: existing answers are not updated, so it is possible to have more than "max items per zone" items dropped into a single zone if "max items" setting changed after the answer was submitted.

Screenshots:

Studio "Zones" tab:

Studio "Items" tab:

LMS - item alignment:

Attempting to exceed max items per zone:

Reviewers:

@itsjeyd
TNL: @cahrens and/or @staubina
~~a11y: @cptvitamin~~ a11y review will be performed later, for all changes in v2.1: SOL-1978
Product: @sstack22

Checklist:

Internal review
Upstream review
Squash commits

openedx-webhooks · 2016-08-09T15:35:07Z

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx-solutions%2Fxblock-drag-and-drop-v2&number=92

itsjeyd · 2016-08-10T09:09:38Z

drag_and_drop_v2/default_data.py

@@ -46,6 +47,8 @@
      "y": 210,
      "width": 340,
      "height": 138,
+      "align": "center"
+


@e-kolpakov Nit: Remove extra blank line

e-kolpakov · 2016-08-10T11:46:43Z

drag_and_drop_v2/public/js/drag_and_drop.js

@@ -708,6 +690,7 @@ function DragAndDropBlock(runtime, element, configuration) {
                        releaseItem($selectedItem);
                    } else if (isActionKey(evt)) {
                        evt.preventDefault();
+                        evt.stopPropagation();


@itsjeyd FYI without this stopPropagation dropping an item using keyboard closed error popup before user could see it, resulting in item being returned to item bank without any explanation why.

@e-kolpakov Thanks for the explanation, I had wondered about that briefly but somehow forgot to comment about it - you read my mind :)

bradenmacdonald · 2016-08-10T17:54:17Z

In this screenshot, the left-aligned items retain a nice small automatic size, but the centered ones grow to fill their zone width - is there a way to use the "left" sizing behavior in both cases?

Also, on the sandbox I wasn't able to see the warning in red when dragging too many items onto a zone.

Finally, can you please enable the XBlock view on the sandbox so we can test how this works on small devices / in the mobile app?

http://studio-dndv2-sandbox9.opencraft.hosting/xblock/block-v1:OpenCraft+DnDv2+demos+type@vertical+block@28ae4b11831b42949d8815c86671424b should work but seems to need FEATURES["ENABLE_XBLOCK_VIEW_ENDPOINT"] set to True.

itsjeyd · 2016-08-10T19:00:06Z

tests/unit/test_basics.py

+        def submission_success(submission):
+            submission['max_items_per_zone'] = 1
+            submission['data']['items'] = [
+                {'zones': ['1'], 'title': 'item 1'}, {'zones': ['2'], 'title': 'item 2'}


@e-kolpakov For consistency with the other submission modification methods, should zones be called 'Zone 1' and 'Zone 2' here as well?

haikuginger · 2016-08-25T20:53:16Z

@itsjeyd, actually, hold off a bit - there's some stuff that automerge didn't catch.

haikuginger · 2016-08-26T09:17:37Z

@itsjeyd, sorry for the delay. This is set up in the sandbox and ready for review.

itsjeyd · 2016-08-26T10:53:11Z

drag_and_drop_v2/utils.py

+        """
+        attributes_to_remove = ['x_percent', 'y_percent', 'left', 'top', 'absolute']
+        for attribute in attributes_to_remove:
+            del[attribute]


@haikuginger This should be del item[attribute], otherwise item will be returned unmodified.

mtyaka · 2016-08-26T11:07:01Z

@itsjeyd @haikuginger I just noticed the state migration related changes in this PR - #98 is somewhat related.

haikuginger · 2016-08-26T11:25:03Z

Well spotted, @itsjeyd. I've switched that to a pop to avoid any possible KeyErrors.

itsjeyd · 2016-08-26T13:00:09Z

drag_and_drop_v2/drag_and_drop_v2.py

@@ -127,6 +129,13 @@ class DragAndDropBlock(XBlock, XBlockWithSettingsMixin, ThemableXBlockMixin):
        default="",
    )

+    max_items_per_zone = Integer(
+        display_name=_("Maximum items per zone"),
+        help=_("This setting limits the number of items that can be dropped into a single zone"),


@haikuginger Please add a period to make this consistent with help messages for other fields, and update the translation files.

itsjeyd · 2016-08-26T13:04:16Z

@haikuginger I tested on the sandbox, and most everything is working as expected. The only issue I found was:

Can't save zone alignment: When opening a DnDv2 instance in Studio after changing the alignment of a zone and submitting the changes, the value is back to "center".

I also had a look at the commits you've pushed so far. They look good (although the commit message of 074a857 is fairly cryptic ;))

haikuginger · 2016-08-26T14:12:51Z

@itsjeyd, I'll investigate that and see what I can figure out. As for the commit message, apparently git does special things with parts of a commit message wrapped in backticks.

haikuginger · 2016-08-26T17:16:14Z

@itsjeyd, those changes are deployed. We seem to be down to pretty minor stuff, so if it's all right, I'll wait to update the sandbox until we've got a 👍.

itsjeyd · 2016-08-29T09:49:20Z

Rebased from a58f72d.

itsjeyd · 2016-08-29T10:55:48Z

@haikuginger Thanks for the updates, they fix the problem. In my capacity as sprint firefighter I took the liberty to push this forward some more: I rebased the branch, cleaned up the test code post-rebase, deployed the updated code to the sandbox, and tested everything again according to the PR description (including behavior for legacy zones with "none" alignment -- this I verified in my local dev env, since it requires working with different branches).

This is ready for upstream review now. 👍

I tested this: Features work as advertised in description
I read through the code
Includes documentation

cahrens · 2016-08-29T14:21:35Z

@itsjeyd Is this PR ready for TNL re-review?

itsjeyd · 2016-08-29T14:54:47Z

Hi @cahrens, yes, this PR is ready for TNL re-review. Thanks!

cahrens · 2016-08-29T20:05:10Z

drag_and_drop_v2/drag_and_drop_v2.py

@@ -315,12 +325,34 @@ def studio_submit(self, submissions, suffix=''):
        self.weight = float(submissions['weight'])
        self.item_background_color = submissions['item_background_color']
        self.item_text_color = submissions['item_text_color']
+        # Possible ValueError should be catched in _validate_submissions


This comment is no longer relevant, correct?

cahrens · 2016-08-29T20:21:18Z

Nit: the following scenario feels a bit odd:

Set maximum items per zone to 2.
Drag 2 items to the zone.
Pick up one of the items and release it (back into the same zone).
You get an error message about not adding more items to the zone.

I wouldn't expect an error message because I'm not adding new items to the zone, just "moving them around".

e-kolpakov · 2016-08-30T12:33:36Z

@cahrens Notes about comments addressed, showing error message when dropping an item to the same zone is fixed, sandbox is updated

sstack22 · 2016-08-30T13:15:57Z

I've finished my review, just one small comment:

I believe the help text for alignment needs to be updated: "Default is no alignment (items stay exactly where the user drops them)." This is no longer true, correct?

cahrens · 2016-08-30T15:11:25Z

👍 on review feedback commits. I still have not reviewed the migration and test code, but I trust your internal reviews.

Please do see @sstack22's comment above though about documentation.

e-kolpakov · 2016-08-31T13:10:16Z

@cahrens @sstack22 rebased from 4beaba2 and fixed help text for zone alignment. Sandbox updated.

sstack22 · 2016-08-31T14:08:29Z

@e-kolpakov - great, confirmed the fix! 👍

* "No alignment" zone option removed * Max items per zone setting added * Zone and state migrations are moved to a dedicated class

e-kolpakov · 2016-09-01T07:34:27Z

Squashed commits - previous revision fabb8f6

itsjeyd reviewed Aug 10, 2016
View reviewed changes

e-kolpakov changed the title ~~(WIP) Prevent overlapping drop placements~~ Prevent overlapping drop placements Aug 10, 2016

e-kolpakov reviewed Aug 10, 2016
View reviewed changes

itsjeyd reviewed Aug 10, 2016
View reviewed changes

itsjeyd reviewed Aug 26, 2016
View reviewed changes

itsjeyd force-pushed the ekolpakov/non-overlapping-placement branch from a58f72d to 68417c3 Compare August 29, 2016 09:49

cahrens reviewed Aug 29, 2016
View reviewed changes

e-kolpakov force-pushed the ekolpakov/non-overlapping-placement branch from 48f4fea to 4beaba2 Compare August 30, 2016 12:08

itsjeyd mentioned this pull request Aug 31, 2016

[SOL-1998] Implement Show Answer button open-craft/xblock-drag-and-drop-v2#10

Merged

e-kolpakov force-pushed the ekolpakov/non-overlapping-placement branch from 4beaba2 to fabb8f6 Compare August 31, 2016 13:09

[SOL-1979] Preventing overlapping item placement + other improvements:

de5ac0c

* "No alignment" zone option removed * Max items per zone setting added * Zone and state migrations are moved to a dedicated class

e-kolpakov force-pushed the ekolpakov/non-overlapping-placement branch from fabb8f6 to de5ac0c Compare September 1, 2016 07:33

e-kolpakov merged commit 4d0d500 into openedx:master Sep 1, 2016

e-kolpakov deleted the ekolpakov/non-overlapping-placement branch September 1, 2016 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent overlapping drop placements #92

Prevent overlapping drop placements #92

e-kolpakov commented Aug 9, 2016 •

edited

Loading

openedx-webhooks commented Aug 9, 2016

itsjeyd Aug 10, 2016

e-kolpakov Aug 10, 2016 •

edited

Loading

itsjeyd Aug 10, 2016

bradenmacdonald commented Aug 10, 2016

itsjeyd Aug 10, 2016

haikuginger commented Aug 25, 2016

haikuginger commented Aug 26, 2016

itsjeyd Aug 26, 2016

mtyaka commented Aug 26, 2016

haikuginger commented Aug 26, 2016

itsjeyd Aug 26, 2016

itsjeyd commented Aug 26, 2016

haikuginger commented Aug 26, 2016

haikuginger commented Aug 26, 2016

itsjeyd commented Aug 29, 2016

itsjeyd commented Aug 29, 2016

cahrens commented Aug 29, 2016

itsjeyd commented Aug 29, 2016

cahrens Aug 29, 2016

cahrens commented Aug 29, 2016

e-kolpakov commented Aug 30, 2016

sstack22 commented Aug 30, 2016

cahrens commented Aug 30, 2016

e-kolpakov commented Aug 31, 2016 •

edited

Loading

sstack22 commented Aug 31, 2016

e-kolpakov commented Sep 1, 2016

Prevent overlapping drop placements #92

Prevent overlapping drop placements #92

Conversation

e-kolpakov commented Aug 9, 2016 • edited Loading

openedx-webhooks commented Aug 9, 2016

itsjeyd Aug 10, 2016

Choose a reason for hiding this comment

e-kolpakov Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

itsjeyd Aug 10, 2016

Choose a reason for hiding this comment

bradenmacdonald commented Aug 10, 2016

itsjeyd Aug 10, 2016

Choose a reason for hiding this comment

haikuginger commented Aug 25, 2016

haikuginger commented Aug 26, 2016

itsjeyd Aug 26, 2016

Choose a reason for hiding this comment

mtyaka commented Aug 26, 2016

haikuginger commented Aug 26, 2016

itsjeyd Aug 26, 2016

Choose a reason for hiding this comment

itsjeyd commented Aug 26, 2016

haikuginger commented Aug 26, 2016

haikuginger commented Aug 26, 2016

itsjeyd commented Aug 29, 2016

itsjeyd commented Aug 29, 2016

cahrens commented Aug 29, 2016

itsjeyd commented Aug 29, 2016

cahrens Aug 29, 2016

Choose a reason for hiding this comment

cahrens commented Aug 29, 2016

e-kolpakov commented Aug 30, 2016

sstack22 commented Aug 30, 2016

cahrens commented Aug 30, 2016

e-kolpakov commented Aug 31, 2016 • edited Loading

sstack22 commented Aug 31, 2016

e-kolpakov commented Sep 1, 2016

e-kolpakov commented Aug 9, 2016 •

edited

Loading

e-kolpakov Aug 10, 2016 •

edited

Loading

e-kolpakov commented Aug 31, 2016 •

edited

Loading