[BB-753] Fix issue with multiple responses for a single user returned by generate_report_data

[xitij2000]

The current problem response code has a bug that causes it to only return a response for each user for each block.

The following code is generating a dictionary keyed on username for the report generated for each block. If a block contains multiple responses per user, then later responses will overwrite older responses.

https://github.com/edx/edx-platform/blob/8593d4dc711c67535a0a0a98fa32b84ce6abc0c1/lms/djangoapps/instructor_task/tasks_helper/grades.py#L674-L678

JIRA tickets: https://openedx.atlassian.net/browse/OSPR-2927

Discussions: TBD

Sandbox URL:

• LMS: https://pr19507.sandbox.opencraft.hosting/
• Studio: https://studio-pr19507.sandbox.opencraft.hosting/

Testing instructions:

1. Generate a problem response report for a block containing multiple questions and responses.
2. The generated report should include all responses.

Author notes and concerns:
Since there are multiple responses for each user state, I am not entirely sure how this should be handled. Currently, it just duplicates the state for each row, however, there may be a good argument for not doing that if it breaks current flows that use the state. (I remember there were concerns when the original PR for this removed the state entirely).

Reviewers

• TBD
• edX reviewer[s] TBD

[openedx-webhooks]

Thanks for the pull request, @xitij2000! I've created OSPR-2927 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.

[natabene]

@xitij2000 Thank you for your contribution, please let me know once it is ready to be looked at.

[xitij2000]

@natabene Will do. I think it's mainly the tests that I am working on. I would like to discuss the possible impact of the changes this could make to the report since I remember there were some concerns before.

[natabene]

@xitij2000 Do you recall who voiced concerns before? Who do you think should review this?

[xitij2000]

@natabene I do know that there were some issues with the removal of the state field from this report. As such this PR was made: https://github.com/edx/edx-platform/pull/18512

This PR will again mess with the report format in a way that might affect people who use this functionality.

The ticket linked there seems to no longer exist, perhaps I got the number wrong since I myself didn't have access to the ticket at that point. @bradenmacdonald might know better.

[xitij2000]

@bradenmacdonald Added this so now a uniform path is returned for all blocks whether you request a report for a course or a single problem.

[natabene]

@bradenmacdonald Please let us know if you have more information.

[bradenmacdonald]

@natabene @xitij2000 You can refer to https://openedx.atlassian.net/browse/EDUCATOR-3143 for information about who was affected last time the report format changed. Unfortunately I don't have permission to view that ticket anymore though?!

[xitij2000]

Yup, I'm having the same issue. Perhaps the ticket is deleted? At the time I made the original PR I don't think I had access to that JIRA at all, so I have no information other than it being an issue at the time.

[natabene]

@xitij2000 @bradenmacdonald Looks like you both had access to internal edX JIRA, and then it was revoked due to contract. I have exported the ticket and sent to both of you via email, i hope this helps!

[xitij2000]

@natabene Thanks!

[xitij2000]

@marcotuts I would love to get your feedback about how I should go about with this fix.
A very short summary:

1. The old problem response report had one row per block with a state that had all the responses for that block.
2. The new enhanced reports can now generate multiple responses per block. The problem block, for instance, might have multiple questions in a single block, each of which would be in an individual row in the enhanced report.
3. We need to include the state object in the report since it's still in active use by people.

The issue now is how to handle cases where there is a single state that corresponds to multiple rows. Two potential solutions are:

• Add a new checkbox to the UI that allows users to request the old style report that has the state. If this is selected, only the state data will be returned. If this is not selected (default) the enhanced report will be generated that only includes the state object for blocks that don't support enhanced response reports.
• Just duplicate the state data across all the rows that correspond to the same state.

In either case, this needs to be fixed since currently the report generator will give inaccurate results, and will only return the first response for blocks that have multiple responses.

[natabene]

@marcotuts Would you be able to help @xitij2000 if you have a chance?

[xitij2000]

There are two more enhancements that we are considering, for the API that initiates the report generation:

1. Add support for filtering by block type, i.e. select the course/a chapter/sequntial etc and select a block type problem-builder, and then it will only generate the reports under the root block for that block type.
2. Add support for specifying multiple root blocks, so you can select perhaps 2 or 3 blocks (or chapters etc) and only reports for starting at those points would be generated.

I can include these enhancements in the same PR if that would be easier.

[natabene]

@xitij2000 Let me ping Marco again. Sorry for delay

[marcotuts]

Hello! Thanks for your patience on this.

My feedback would be to add the field to all cases, meaning this option:
"Just duplicate the state data across all the rows that correspond to the same state."

We may also want to update documentation here if this applies / exists, and perhaps an ADR makes sense to explain why the state field still exists, but it could potentially be flagged for deprecation via DEPR ticket in a future named release?

For these items, I think we could look at a separate PR / review for these two additions:

• Add support for filtering by block type, i.e. select the course/a chapter/sequntial etc and select a block type problem-builder, and then it will only generate the reports under the root block for that block type. (Nice To Have, exclude from this PR - Marco)
• Add support for specifying multiple root blocks, so you can select perhaps 2 or 3 blocks (or chapters etc) and only reports for starting at those points would be generated. (Nice To Have, exclude from this PR - Marco)

[xitij2000]

@marcotuts Thanks for the feedback! Will update the PR accordingly.

[natabene]

@xitij2000 Thank you. @ormsbee Could you take a look?

[ormsbee]

I have some minor nits, questions, and a request for a little more explanation in the code. As always, feel free to ignore the "Nit:" items.

My biggest concern is what this means in terms of backwards compatibility. Could you please do a short example of before and after behavior? Is the output the same as it was before for cases where there is only one response?

[ormsbee]

Nit: Using [::-1] is concise, but a lot of folks aren't familiar with that notation. Please use something like return list(reversed(path)) or something like that.

[ormsbee]

Also, .display_name is not guaranteed to exist for container types that exist below the level of Verticals. LibraryContentModule and SplitTestModule do define it, but there might be third party things that don't, so it might be worthwhile to have a fallback.

[ormsbee]

Nit: This is fine as is, but I find collections.defaultdict slightly easier to read:

generated_report_data = defaultdict(list)
for username, state in block.generate_report_data(user_state_iterator, max_count):
    generated_report_data[username].append(state)

This is a stylistic choice and completely optional. I just wanted to mention it in case you weren't familiar with defaultdict.

[ormsbee]

Please insert a comment explaining what is going on in this block?

[ormsbee]

Is building this path necessary to fix the bug? It's convenient, but it seems like a format change that might break client expectations?

[xitij2000]

It is not necessary for this bug, but I thought it would be a small useful improvement. It is a backwards-incompatible change, but I think it makes the paths more consistent. One main benefit I see here is when you need to combine two reports, the paths are more useful.

I can definitely remove it, or move it to my other PR (https://github.com/edx/edx-platform/pull/19781) that deals with enhancements to this code.

[ormsbee]

If the goal is to get the bugfix into Ironwood, then I think we should go for as much compatibility as possible, and put enhancements into a separate PR (either #19781 or something else).

[xitij2000]

@ormsbee The response will be the same in case there is only a single response for a block. In cases where there are multiple responses for a block, the situation is different.

For people used to the Ginkgo report format, this change will now break the assumption that there is one row per block (if there was such an assumption, to begin with). However, on a per-row level, it has all the information they need, i.e. username, and state.

(I will post a before and after sample soon)

[ormsbee]

@xitij2000: Okay, that sounds fine for the altar of backwards compatibility. Please split out the path enhancement to a different PR and we'll get this bugfix merged and cherry-picked to Ironwood.

Thank you,

[xitij2000]

@ormsbee I'm making it fall back to the 'category' in case there is no display name. I hope that works.

[xitij2000]

jenkins run bokchoy

[ormsbee]

As usual, please squash and include context from the PR message into the commit message. After that, I'll merge. Thank you!

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[xitij2000]

@ormsbee I've squashed down to one commit and updated the commit message.

[openedx-webhooks]

@xitij2000 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[nedbat]

I have cherry-picked this onto Ironwood.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, March 18, 2019.

[edx-pipeline-bot]

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/STAGE_edxapp_M-D

[xitij2000]

@nedbat Thanks!

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.