[BB-873] Support for filters, and multiple roots in problem response reports - #21413
Conversation
|
Thanks for the pull request, @xitij2000! I've created OSPR-3799 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:
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. |
openedx-webhooks
added
needs triage
open-source-contribution
There was a problem hiding this comment.
👍
- I tested this:
- Checked that python tests are passing locally ✔️
- Performed the same testing as in the previous PR (https://github.com/edx/edx-platform/pull/19781#discussion_r312544842).
- Logged in as staff and answered some problems on the demo course.
Generate problem response reports for 2 problems sending comma-separated values. I used these:
block-v1:edX+DemoX+Demo_Course+type@problem+block@d2e35c1d294b4ba0b3b1048615605d2a
block-v1:edX+DemoX+Demo_Course+type@problem+block@75f9562c77bc4858b61f907bb810d974
- Checked that the report was correctly generated and responses for both problems were included ✔️
- Checked that we can generate a problem response report for the entire course with
block-v1:edX+DemoX+Demo_Course+type@course+block@course✔️
5.Used section block-v1:edX+DemoX+Demo_Course+type@course+block@course and tested the problem_types_filter with three values:
openassessment
problem
openassessment,problem
- Checked that the generated reports contained only the problems that were specified on each operation heavy_check_mark
- I read through the code
- Includes documentation: doesn't include documentation
⚠️ -
I checked for accessibility issuesNA
@xitij2000 Thanks for fixing this bug and creating this again! Nice work! 😁
I used the same testing method as before, but changed the problems I used.
openedx-webhooks
added
waiting on author
|
@xitij2000 Thank you for your contribution. Please let me know once it is ready for our review. |
6694cd4 to
5a8012b
Compare
|
@natabene This is ready. |
|
@marcotuts Do you want to review this before it goes to engineering? |
openedx-webhooks
added
product review
|
This can move to @ormsbee next as it is a follow-on to a previous contribution. Thanks! |
openedx-webhooks
added
awaiting prioritization
and removed
product review
5a8012b to
08a9ec4
Compare
|
jenkins run all |
b8ba240 to
91ead21
Compare
39cc9bb to
40c3245
Compare
openedx-webhooks
added
open edx community review
and removed
awaiting prioritization
labels
|
@bradenmacdonald This is ready for review. |
40c3245 to
69c50e2
Compare
69c50e2 to
cfebd4e
Compare
cfebd4e to
9be0a0a
Compare
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
👍 My only remaining concern is that the filenames generated with comma-separated xblock IDs in them can get absurdly long. A nice future improvement would be to limit the filename length to something reasonable.
- I tested this: using the sandbox as described. Test method:
const apiUrl = 'https://pr21413.sandbox.opencraft.hosting/courses/course-v1:edX+DemoX+Demo_Course/instructor/api/get_problem_responses';
let result;
result = await $.post(apiUrl, {'problem_location': 'block-v1:edX+DemoX+Demo_Course+type@problem+block@Sample_Algebraic_Problem'})
result = await $.post(apiUrl, {'problem_location': 'block-v1:edX+DemoX+Demo_Course+type@problem+block@Sample_Algebraic_Problem,block-v1:edX+DemoX+Demo_Course+type@problem+block@a0effb954cca4759994f1ac9e9434bf4'})
result = await $.post(apiUrl, {'problem_location': 'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations', 'problem_types_filter': 'problem'})
# From what I can tell, the sample course on the sandbox doesn't have anything other than 'problem' blocks that would show up in this report? So I just tested with html, which made an empty report
result = await $.post(apiUrl, {'problem_location': 'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations', 'problem_types_filter': 'html'})
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation - yes, in comments. Should really be in the
get_problem_responsesdocstring too though.
4a86a97 to
fb6c118
Compare
I've added docs there now.
Would it make sense to only use the following logic for the name:
|
|
@xitij2000 That logic sounds good. Would that keep the filenames unchanged for existing reports? I'm not sure that matters but is probably a good requirement to stick to, if for nothing else other than to not break organization on the file system for people who regularly download these reports and save them into a local folder. |
|
@bradenmacdonald Currently only a single block at a time is supported by the API and UI, and for that case, the file name will remain unchanged. The new file naming logic will only kick in for multiple blocks, or when filters are used. Currently, there is no way to initiate such reports from the UI so until that happens, there is no easy way to generate such reports anyway. |
|
@xitij2000 Great. I'm unclear on the status now: are you ready to merge this (if so please squash), or are you going to push a commit to add file name shortening? I'm fine either way. |
ecb848d to
8f29501
Compare
|
@bradenmacdonald I've just added a commit to add better file-name handling and tests for it. |
8f29501 to
ac23167
Compare
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Great, thanks! My approval still stands. The latest commit looks good. Please squash and we'll get this merged.
…ks, or filtered block types. This change adds support for specifying multiple root blocks while generating problem response reports. It also allows specifying a block type filter so that only blocks of the filtered types will be included in the report. Finally, this change also consistenly uses absolute path for the location in the report instead of relative paths.
4478ff3 to
2eff127
Compare
|
jenkins run all |
|
Your PR has finished running tests. There were no failures. |
|
@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. |
openedx-webhooks
added
open edx community review
merged
and removed
needs triage
open edx community review
labels
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
9 participants
Add support for generating problem response reports for multiple blocks, or filtered block types.
This change adds support for specifying multiple root blocks while generating problem response reports. It also allows specifying a block type filter so that only blocks of the filtered types will be included in the report.
Finally, this change also consistently uses absolute path for the location in the report instead of relative paths.
This redoes #19781 which had a glaring issue introduced during refactoring.
JIRA tickets: If there is a related publicly viewable JIRA ticket, mention that here. Also mention the OSPR ticket
number here once it gets created (after you open the PR), if applicable. Otherwise omit this.
Discussions: https://github.com/edx/edx-platform/pull/19507
Dependencies: None
Sandbox URL:
Merge deadline: None
Testing instructions:
Reviewers