BB-726 Implement generate_report_data for PollBlock and SurveyBlock #50

john2x · 2018-12-19T09:28:25Z

See BB-726

Example outputs:

Poll

username,title,location,Answer,Answer ID,Question,block_key,state
verified,Color Poll,Polls > Colors > Color Poll,Red,R,What is your favorite color?,block-v1:John+JL02+2018+type@poll+block@e660a593f8dc40b891283038838bd73b,"{""submissions_count"": 1, ""choice"": ""R""}"
verified,Fruits Poll,Polls > Fruits > Fruits Poll,Durian,1545204793464,What is your favorite fruit?,block-v1:John+JL02+2018+type@poll+block@d83bd8fad2fe416a922ee2fcd7b047c8,"{""submissions_count"": 1, ""choice"": ""1545204793464""}"

Survey

username,title,location,Answer,Answer ID,Question,Question ID,block_key,state
verified,Survey,Surveys > Course feedback > Survey,Maybe,M,Do you think you will learn a lot?,learn,block-v1:John+JL02+2018+type@survey+block@f8a185f62a7a4d01bf349eb1ce977fef,"{""submissions_count"": 1, ""choices"": {""enjoy"": ""Y"", ""learn"": ""M"", ""recommend"": ""N""}}"
verified,Survey,Surveys > Foods > Survey,Eh,M,Do you like vegetables?,learn,block-v1:John+JL02+2018+type@survey+block@d1bf4def19dc4e5a9f7e5580893cf657,"{""submissions_count"": 1, ""choices"": {""enjoy"": ""Y"", ""learn"": ""M"", ""recommend"": ""Y""}}"

TODO

try with polls/surveys that allow for multiple responses
translations
tests

See BB-726

xitij2000

This is a great start! I've pointed out some issues. Maybe you already thought of some or all of them :)

poll/poll.py

xitij2000 · 2018-12-19T10:44:06Z

poll/poll.py

+                    self.ugettext("Answer ID"): a,
+                    self.ugettext("Answer"): answer
+                }
+                yield (user_state.username, report)


I think this will end up creating multiple responses per student, which is not the way to go here. The report generator creates a dict that maps from the username to the report, if it gets multiple reports per username, they will get merged. So this will not work.

All the questions and answers here should be merged into a single report. For instance you could have the key for the question be: 'Question ({})'.format(question_id)

@bradenmacdonald How do you think this should be handled? Do you think it makes sense to enhance the current reporting code to support cases where we have multiple responses for a single user for a single block?

@xitij2000 Do you mean things like Capa problems that can have multiple questions in a single block? I thought we're handling that already. The capa generate_report_data code definitely looks like it yields multiple rows per block per student when appropriate.

@bradenmacdonald I think then we might have a bug because of this code here:
https://github.com/edx/edx-platform/blob/055f7323f7f0c5389aa81dd46935499310b0e258/lms/djangoapps/instructor_task/tasks_helper/grades.py#L673-L678

It's creating a dict keyed on the username, so if it gets two rows with the same username, it will only keep one. I did a quick test and this seems to be the case.

Given that, I think the code @john2x wrote is correct, but we need to fix the report generator. This will also need some additonal thought because it means we now have multiple responses with the same block key and state and I don't know how that will interfere with existing tools. Some people were affected when the state column was removed.

Thanks @xitij2000. You are right about the other answers overwriting the previous ones. This is actually apparent in my example output above (there should've been 6 rows). I missed that.

I can flatten the questions for the Survey report like 'Question ({})'.format(question_id) for the meantime to fit the current expectations of the report generator. Maybe something like the following (question ids are enjoy and learn)?

username,...,Question (enjoy),Answer ID (enjoy),Answer (enjoy),Question (learn),Answer ID (learn),Answer (learn),... john,...,Are you enjoying the course?,Y,Yes,Do you think you will learn a lot?,M,Maybe,...

Though for the Capa report, there doesn't seem to be question id's that can be used for the header. We could use the question label/text itself for the headers? But that might get too long.

How much work will it be to open a fix PR for making the platform's report generator accept multiple entries? Then once we have that PR open we can ask edX + the community if it will break any tooling.

I don't think it's much work since I know exactly what to do. The only issue would be the duplicated state entries.

One option would be to add a checkbox or similar to the UI that lets people switch between raw reports or enhanced reports. If they unselect enhanced reports (default is selected) then we only output the state in old-style reports.

Is it worth posting this in slack? Or perhaps the forum? I'll make a PR first and link to that either way.

@john2x Yup, that is exactly what I was going for :-) No other places would be affected other than the report, so this is a "safe" change to make.

Thanks @xitij2000 :) So just to confirm, we'll keep the Survey report to yield multiple answers per username?

I've prepared changes to flatten the survey report

for user_state in user_state_iterator: if limit_responses and count >= limit_responses: # End the iterator here return # user_state.state={'submissions_count': 1, 'choices': {u'enjoy': u'Y', u'recommend': u'N', u'learn': u'M'}} choices = user_state.state['choices'] # flatten the multiple questions/answers in a survey into one report row report = {} for q, a in choices.items(): question = questions_dict[q]['label'] answer = answers_dict[a] report.update({ self.ugettext('Question ({question_id})'.format(question_id=q)): question, self.ugettext('Answer ID ({question_id})'.format(question_id=q)): a, self.ugettext('Answer ({question_id})'.format(question_id=q)): answer, self.ugettext('Submissions count ({question_id})'.format(question_id=q)): user_state.state['submissions_count'] }) if report: count += 1 yield (user_state.username, report)

@john2x Yeah, this will be fixed in the generate method.

poll/poll.py

xitij2000

👍 This is looking good and working as expected. I just have a few minor nits about the code that I'd like to see addressed before this is merged.

I tested this: tested on devstack
I read through the code
[na] I checked for accessibility issues
[na] Includes documentation

xitij2000 · 2018-12-24T08:14:34Z

poll/poll.py

+            })
+        """
+        if limit_responses == 0:
+            return


I'm not sure why this is necessary. It will end with line 813 anyway.

Improved the limit_responses check (both here and in survey report). Removed this block and changed the condition in the loop to check if limit_responses is not None. Also added tests for limit_responses=0.

poll/poll.py

xitij2000 · 2018-12-25T17:57:02Z

tests/unit/test_xblock_poll.py

+        # each choice of a user gets its own row
+        # so the first three rows should be edx's choices
+        self.assertEqual(['edx', 'edx', 'edx'],
+                         [username for username, _ in report_data[:3]])


Could you bump this up to 4 so we get responses for 2 users?

Updated to assert for 2 users

xitij2000 · 2018-12-25T17:57:56Z

tests/unit/test_xblock_poll.py

+        # each choice of a user gets its own row
+        # so the first two rows should be edx's choices
+        self.assertEqual(['edx', 'edx'],
+                         [username for username, _ in report_data[:3]])


nit: I think the [:3] is a bit distracting here since it's doing nothing.

Removed unnecessary [:3]

xitij2000 · 2018-12-25T18:01:46Z

poll/poll.py

+            })
+        """
+        if limit_responses == 0:
+            return


This can be removed, same comments as above.

xitij2000 · 2018-12-25T18:02:19Z

poll/poll.py

+        for user_state in user_state_iterator:
+            # user_state.state={'submissions_count': 1, 'choices': {u'enjoy': u'Y', u'recommend': u'N', u'learn': u'M'}}
+            choices = user_state.state['choices']
+            for q, a in choices.items():


Please use more descriptive names than q and a, like question_id and answer_id.

Updated variable names

xitij2000 · 2018-12-25T18:03:08Z

@john2x A couple of general comments:

I think you can remove Question ID and Answer ID from everywhere.
You should add a version bump here, a minor bump should do.

- Remove Question ID and Answer ID from reports - Improve limit_responses check - Increase survey test to check for 4 items instead of 3

xitij2000 · 2018-12-25T23:51:41Z

setup.py

@@ -45,7 +45,7 @@ def package_data(pkg, roots):

 setup(
    name='xblock-poll',
-    version='1.6.3',
+    version='1.6.4',


nit: could you bump this to 1.7 since it's adding a new feature?

After bumping it you can squash the commits and merge it.

Thanks @xitij2000. I've bumped to 1.7.0 and squashed and merged.

Implement generate_report_data methods for Poll and Survey blocks

a7c047e

See BB-726

john2x force-pushed the john2x/BB-726-generate-report-data branch from 8f46400 to a7c047e Compare December 19, 2018 09:42

xitij2000 reviewed Dec 19, 2018

View reviewed changes

john2x changed the title ~~Implement generate_report_data for PollBlock and SurveyBlock~~ BB-726 Implement generate_report_data for PollBlock and SurveyBlock Dec 19, 2018

john2x added 4 commits December 20, 2018 05:34

Stop iterator when limit_responses is reached

bf6e63f

Include submissions count in Poll report data

a8181c0

Add unit tests for generate_report_data

3ee9bbd

Add translations

25729df

john2x force-pushed the john2x/BB-726-generate-report-data branch from d1ab927 to 25729df Compare December 21, 2018 02:46

xitij2000 approved these changes Dec 25, 2018

View reviewed changes

xitij2000 reviewed Dec 25, 2018

View reviewed changes

poll/poll.py Outdated

})

"""

if limit_responses == 0:

return

Copy link

Member

xitij2000 Dec 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed, same comments as above.

xitij2000 reviewed Dec 25, 2018

View reviewed changes

john2x added 3 commits December 26, 2018 05:12

Address PR comments

d9f46c1

- Remove Question ID and Answer ID from reports - Improve limit_responses check - Increase survey test to check for 4 items instead of 3

Version bump to 1.6.4

46e7a73

Update translations

9198f56

john2x force-pushed the john2x/BB-726-generate-report-data branch from a5a905a to 9198f56 Compare December 25, 2018 21:43

xitij2000 reviewed Dec 25, 2018

View reviewed changes

Bump to 1.7.0

ca488ed

john2x merged commit 291dce0 into master Dec 26, 2018

moeez96 mentioned this pull request Feb 7, 2019

Mckin 9196 - Provide human readable user interactions to new instructor tool mckinseyacademy/xblock-scorm#24

Merged

xitij2000 deleted the john2x/BB-726-generate-report-data branch June 24, 2019 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BB-726 Implement generate_report_data for PollBlock and SurveyBlock #50

BB-726 Implement generate_report_data for PollBlock and SurveyBlock #50

john2x commented Dec 19, 2018 •

edited

Loading

xitij2000 left a comment

xitij2000 Dec 19, 2018

bradenmacdonald Dec 19, 2018

xitij2000 Dec 21, 2018 •

edited

Loading

john2x Dec 21, 2018 •

edited

Loading

bradenmacdonald Dec 21, 2018

xitij2000 Dec 21, 2018

xitij2000 Dec 21, 2018

john2x Dec 21, 2018

john2x Dec 23, 2018

xitij2000 Dec 24, 2018

xitij2000 left a comment

xitij2000 Dec 24, 2018

john2x Dec 25, 2018 •

edited

Loading

xitij2000 Dec 25, 2018

john2x Dec 25, 2018 •

edited

Loading

xitij2000 Dec 25, 2018

john2x Dec 25, 2018

xitij2000 Dec 25, 2018

xitij2000 Dec 25, 2018

john2x Dec 25, 2018

xitij2000 commented Dec 25, 2018

xitij2000 Dec 25, 2018

xitij2000 Dec 25, 2018

john2x Dec 26, 2018

BB-726 Implement generate_report_data for PollBlock and SurveyBlock #50

BB-726 Implement generate_report_data for PollBlock and SurveyBlock #50

Conversation

john2x commented Dec 19, 2018 • edited Loading

Example outputs:

TODO

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

john2x Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john2x Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john2x Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 commented Dec 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john2x commented Dec 19, 2018 •

edited

Loading

xitij2000 Dec 21, 2018 •

edited

Loading

john2x Dec 21, 2018 •

edited

Loading

john2x Dec 25, 2018 •

edited

Loading

john2x Dec 25, 2018 •

edited

Loading