Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SOL-1805] Add "item" field to item.dropped event #9

Merged
merged 1 commit into from
Sep 12, 2016

Conversation

arbrandes
Copy link

@arbrandes arbrandes commented Aug 19, 2016

Description

Adds a new "item" field to the edx.drag_and_drop_v2.item.dropped event data. It contains the item text label when present, otherwise the item imageURL. This makes it consistent with the location field, which contains the zone text label.

Dependencies

None

JIRA

Sandbox URLs

  • LMS: a predeployed DnDv2 instance
  • Event log: a constantly updated event log, filtered for the item.dropped event.

Latest commit

Testing instructions

  1. Go to the pre-deployed DnDv2 instance on the LMS, and log in as staff@example.com.
  2. Pick any item and drop it onto any zone.
  3. Open the event log URL, refresh it (with F5 or equivalent) and scroll to the last line.
  4. Look for the "event": {} part of the line (it helps to CTRL-f), and check that there is an item element with a value that matches the label of the item you manipulated above.

@arbrandes
Copy link
Author

@bradenmacdonald, can you please merge edx-solutions@master into open-craft@master, so as to clear up this PR? :) Thanks!

@bradenmacdonald
Copy link
Member

@arbrandes Done; sorry for the delay. GitHub doesn't seem to have noticed, though...

@pomegranited
Copy link
Member

Hi @arbrandes ! I'm reviewing your PR, and it's looking great! Just have a couple issues/questions/notes before I approve and create an upstream PR.

  • I tested this by installing this PR's branch on my devstack, publishing a DnDv2 unit in my test course, and watching the tracking logs while I interacted with it. Tested with text and image items.
  • I read through the code.
  • Includes documentation, and the additional location documentation as specified by SOL-1805.
  1. Please fix the these minor issues travis found: pep8 and pylint
  2. I've tested your change on the sandbox instance, and was able to see the item field appear fine in the /edx/var/log/tracking/tracking.log file, but not in the event log URL link you provided in the description. Is there a cron or something that's supposed to keep this file updated?
  3. Out of curiosity, has there been any discussion about updating the item.picked_up event too? It seems strange to leave that one with just the item_id.
  4. FYI, I added a new item to your sample case that uses an imageURL and no item text, and it tested out fine.

@arbrandes
Copy link
Author

@pomegranited, thanks for the review! I've addressed the issues and amended the PR.

As for the event log URL, that's a simple tail -f | grep on the tracking log file. It looks like it was rotated since first starting it, so that would explain it not working for you. I restarted it, and it should work - for a while, at least. :)

@arbrandes
Copy link
Author

@pomegranited, forgot to answer your question: I'm not aware of any discussion of modifying item.picked_up in a corresponding fashion. @bradenmacdonald, what should we do?

@bradenmacdonald
Copy link
Member

@arbrandes If it's an easy fix, I think we should modify it in the corresponding way.

@arbrandes
Copy link
Author

@bradenmacdonald, after a quick investigation, the item.picked_up event is generated browser-side, as opposed to item.dropped, which is generated server-side. Plus, it doesn't look like the necessary data is cleanly available for inclusion in the event, which points to a possible refactor. Thus, I suggest handling item.picked_up in a separate PR.

@bradenmacdonald
Copy link
Member

@arbrandes Ok, let's leave it for now. Thanks.

@pomegranited
Copy link
Member

Thanks, @arbrandes ! The Travis CI is looking fine now, but CircleCI is failing, for what look like really fundamentall module loading reasons? @bradenmacdonald , is this maybe an issue with our fork's CircleCI configuration?

@bradenmacdonald
Copy link
Member

@pomegranited CircleCI is just an experimental thing, I think started briefly by @haikuginger who never finished getting it working. So we're only really using Travis for this repo.

@pomegranited
Copy link
Member

pomegranited commented Aug 25, 2016

Sweet, thanks for the info, @bradenmacdonald.

Your updated code checks out, @arbrandes ! 👍

I've created edx-solutions PR #97, and pinged upstream for review.

Adds a new "item" field to the edx.drag_and_drop_v2.item.dropped event
data. It contains the item text label when present, otherwise the item
imageURL.

This makes it consistent with the location field, which contains the
zone text label.
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