search

mitodl / edx-platform

The Open edX platform, the software that powers edX!
http://open.edx.org/
GNU Affero General Public License v3.0
6 stars 1 forks source link

Fix capa report generation issue #264

Closed HamzaIbnFarooq closed 3 years ago

HamzaIbnFarooq commented 3 years ago

Description

Fixes Capa questions report generation issues by providing a default value for the problems having no text values for answer options.

Related Issues

https://github.com/mitodl/edx-platform/issues/262 https://github.com/mitodl/edx-platform/pull/239

How to test?

Test 1:

Generate the issue without this branch's fix: 1) Create a Checkbox Problem on studio 2) Don't put any text against one of the options image 3) Through LMS select that option with no text 4) Go to Data Download under Instructor tab 5) Under the Reports section, select the Problem and press Create a report of problem responses The error will be generated.

Now use this branch and try downloading the report again, it should be fixed now.

Test 2:

Generate the issue without this branch's fix: 1) Create a checkboxes problem 2) Add N options under that problem (where N is a number) 3) Go to LMS, select and submit Nth option 4) Go back to the studio and update the question by removing the Nth option (now the total options will be N-1) 5) Try generating a response report for that problem, it will give Error: There was an error generating your report. error.

Now use this branch and try downloading the report again, it should be fixed now.

HamzaIbnFarooq commented 3 years ago

I'd recommend you add units tests too so we can have a single upstream PR.

@ziafazal we are not planning this PR to be an upstream one, we will be using this locally right now. We will be creating an upstream of https://github.com/mitodl/edx-platform/pull/265 only, once that gets reviewed.

HamzaIbnFarooq commented 3 years ago

@pdpinch Do you think we should create an upstream PR of fixing old data for capa reports generation and add markdown validation in capa problems combined OR stick with the original plan as mentioned here saying that only add markdown validation in capa problems should be upstream.

pdpinch commented 3 years ago

My first priority is to get the Residential MITx teams access to their data, but yes we should plan on upstreaming both of these fixes to edx master.

@pdpinch https://github.com/pdpinch Do you think we should create an upstream PR of fixing old data for capa reports generation https://github.com/mitodl/edx-platform/pull/264 and add markdown validation in capa problems https://github.com/mitodl/edx-platform/pull/265 combined OR stick with the original plan as mentioned here https://github.com/mitodl/edx-platform/pull/239#issuecomment-849853504 saying that only add markdown validation in capa problems https://github.com/mitodl/edx-platform/pull/265 should be upstream.

HamzaIbnFarooq commented 3 years ago

My first priority is to get the Residential MITx teams access to their data, but yes we should plan on upstreaming both of these fixes to edx master.

@pdpinch you can have a look at the current PR, if it seems good to you then we can merge it right away. I can cherry-pick the commits to create an upstream PR and add some tests afterwards.

pdpinch commented 3 years ago

@mitodl/devops can we try deploying this change to lms.mitx.mit.edu to see if it helps address ZenDesk 96001

blarghmatey commented 3 years ago

Once it's merged we can run a build to get this through to production. Code looks good to me :+1:

HamzaIbnFarooq commented 3 years ago

@blarghmatey the current branch's fix was removed from mitx/koa most probably due to a force push (maybe yesterday), so whenever it's feasible for you, can you push the commit back to mitx/koa to reflect the changes?