open-craft / xblock-poll

An XBlock for Polling users and displaying the result
GNU Affero General Public License v3.0
18 stars 66 forks source link

CSV export for LMS #35

Closed ny0m closed 7 years ago

ny0m commented 7 years ago

Adds CSV export + download buttons to the Survey and Poll XBlocks.

JIRA tickets: OC-2981 MCKIN-5433

Testing instructions:

  1. In a course with a survey or poll xblock,
  2. Click on the export button to queue a CSV export
  3. Click the now-active download button to download the CSV
  4. Notice the difference in formatting between survey and poll output

Screenshot:

screen shot 2017-09-15 at 4 10 24 pm

Reviewers

cgopalan commented 7 years ago

@bradmerlin Will the "export" and "download" buttons be only visible until the survey and poll are unanswered in the LMS? They disappear as soon as the results are submitted. Also when I try to download the csv for a survey by clicking the button I get this:

http://localhost:8000/media/5a0525aab94db835a763cf62557ec6c79c7ae04f/survey-data-export-2017-09-23-235733.csv
"/edx/var/edxapp/uploads/5a0525aab94db835a763cf62557ec6c79c7ae04f/survey-data-export-2017-09-23-235733.csv" does not exist
bradenmacdonald commented 7 years ago

@cgopalan That error is normal on devstacks, but see if the file actually exists within the devstack at the given file path.

cgopalan commented 7 years ago

@bradenmacdonald @bradmerlin I did not see a file in that path. I realized it was because this uses celery and I had not started celery. When I started celery, it seems that it does not recognize the new task added here export_csv_data. I started celery using this: paver devstack lms --settings=devstack_with_worker and ./manage.py lms celery worker --settings=devstack_with_worker. What am I doing wrong?

ny0m commented 7 years ago

@cgopalan You aren't doing anything wrong – you'll need to add 'poll' to your edx-platform's OPTIONAL_APPS – I've created a PR for that here.

ny0m commented 7 years ago

@cgopalan and with regards to this:

Will the "export" and "download" buttons be only visible until the survey and poll are unanswered in the LMS? They disappear as soon as the results are submitted.

I chose to go with the decision made for the 'View Results' button. I agree that it's a bit confusing – I assumed there might have been some reason for it.

ny0m commented 7 years ago

@cgopalan I've improved the UI in this commit:

cgopalan commented 7 years ago

@bradmerlin some errors from testing:

  1. If survey results have not been submitted yet, and I export, the file is shown like this:

    user_id,username,user_email,Are you enjoying the course?,Do you think you will learn a lot?,Would you recommend this course to your friends?
    14,cgopalan,chandrakant@opencraft.com
  2. If I click export on one of the poll/surveys, the buttons change for both as shown in screenshot below:

screen shot 2017-09-27 at 1 19 37 pm
cgopalan commented 7 years ago

@bradmerlin thanks for fixing the above 2 errors. I verified that they work now. 2 more observations:

  1. Should the download/export work in studio? The buttons are visible but when I click "Export" I get this error: "'Settings' object has no attribute 'GRADES_DOWNLOAD'"
  2. If the user hits "Export" , the export button is disabled and the "download" button is enabled. But if the user has not submitted the answer yet, and submits and then wants to export again, the export button is disabled. Maybe the export button should be enabled when user hits submit?
ny0m commented 7 years ago

@cgopalan It's not meant to run in Studio – GRADES_DOWNLOAD is only available in LMS. I think that people getting results would only be staff-level users, so they would probably not need to export.

cgopalan commented 7 years ago

@bradmerlin is there a way to hide them in studio?

ny0m commented 7 years ago

@cgopalan I think what's visible in studio is just meant to be a visual demo – right?

bradenmacdonald commented 7 years ago

@bradmerlin The Problem Builder Instructor Tool has an example of showing different messaging within Studio vs. the LMS

cgopalan commented 7 years ago

@bradmerlin let me know when you have addressed the above 2 points and I will review again

cgopalan commented 7 years ago

@bradenmacdonald @bradmerlin just wanted to make sure - is it the right practice to hide the buttons that are non-functional in studio? Or do something else, like show a message?

ny0m commented 7 years ago

@cgopalan Done and done!

cgopalan commented 7 years ago

@bradmerlin I have tested your changes and they are working great! One thing I observed is that when I view the page as non-staff (student), it shows the message from studio "Student data and results CSV available for download in LMS.". See screenshot below. Seems kinda strange to refer to LMS while in LMS. Maybe the message needs to be changed? Should it even be displayed to non-staff? Any other ideas?

screen shot 2017-09-28 at 12 21 26 pm

If we can decide what to do with this and implement it, I am good with approving this.

cgopalan commented 7 years ago

:+1:

@bradmerlin this looks good. Once you squash remove the extra blank line in the test file and squash the commits, I can merge.

@bradenmacdonald any concerns from your end?

ny0m commented 7 years ago

@cgopalan Although the extra blank line doesn't add any functionality, it does improve the file's style.

bradenmacdonald commented 7 years ago

@bradmerlin The UI looks nice, and you've done a great job integrating it into Studio and the LMS and showing it only to staff users.

However, Before I had added poll to OPTIONAL_APPS and spawned a worker: When I click "Export Results to CSV", the block immediately enables the "Download CSV" button, even though the CSV is not ready - and the link it has is of course invalid. I would expect it to show an error message instead, or at least wait forever for the CSV to be generated, with both the "Export Results to CSV" and "Download CSV" buttons disabled until the CSV has been successfully generated, and then the "Download CSV" button becoming enabled.

Also, even when I did add poll to my OPTIONAL_APPS, I'm still not able to see an actual CSV get generated. The celery worker doesn't seem to start any new task when I click the "Export Results to CSV" button. Any idea what I'm missing?

cgopalan commented 7 years ago

@bradenmacdonald where are you checking for the CSVs? It should be in /tmp/edx-s3/grades/<id>/

bradenmacdonald commented 7 years ago

@cgopalan The folder /tmp/edx-s3/ doesn't even exist on my devstack - am I missing something?

cgopalan commented 7 years ago

@bradenmacdonald its created when you hit "Export".

bradenmacdonald commented 7 years ago

@cgopalan Well, I did click that...

cgopalan commented 7 years ago

@bradenmacdonald strange, its working for me. As soon as I click 'export', I can see a directtory and the file created like this:

edxapp@vagrant:~/edx-platform$ ls -alt /tmp/edx-s3/grades/5a0525aab94db835a763cf62557ec6c79c7ae04f/
total 12
drwxrwxr-x 2 edxapp edxapp 4096 Oct  2 16:06 .
drwxrwxr-x 3 edxapp edxapp 4096 Oct  2 16:06 ..
-rw-rw-r-- 1 edxapp edxapp  187 Oct  2 16:06 survey-data-export-2017-10-02-160625.csv
bradenmacdonald commented 7 years ago

Sorry for the false alarm - I just tried again and it seems to be working now :)

There is one issue though: The CSV export uses the auto-generated option IDs, which the user cannot control.

user_id,username,user_email,question,answer
5,staff,staff@example.com,What is your favorite color?,G
14,user1examplecom,user1@example.com,What is your favorite color?,G

Under the "answer" column, it should say "Green", not "G", since there is no easy way for course authors/administrators to view/edit the relationship between the answer IDs ("G") and the answer values ("Green"). So for any poll with a non-default question and options, the values in the answer column will be pretty much meaningless.

Otherwise, 👍 from me and I'll let @cgopalan give the final approval once the above is addressed, as I won't be around.