XBlock Reporting: Display a popup after requesting a report #18244

akovari · 2018-05-22T09:02:56Z

This feature implements a popup for generating / downloading the xblock report. It will poll for the generation task status and after it's completed, it will show a button to download the report directly in the modal window.

Dependencies: https://github.com/edx/edx-platform/pull/18170

Screenshots:

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: None

Testing instructions:

Make sure ENABLE_GRADE_DOWNLOADS is enabled in lms/djangoapps/instructor/settings/common.py
Go to Instructor Dashboard -> Data Downloads
In section "Select a problem to generate a CSV file that lists all student answers to the problem. You also select a section or chapter to include results of all problems in that section or chapter."

Pick a course block
Click "Create a report of problem responses"
Observe the modal window

Reviewers

xitij2000
edX reviewer[s] TBD

openedx-webhooks · 2018-05-22T09:03:04Z

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

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

xitij2000 · 2018-05-22T12:44:59Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.jsx

@@ -32,6 +32,7 @@ export default class Main extends React.Component {
                <BlockBrowserContainer onSelectBlock={(blockId) => {
                    this.hideDropdown();
                    onSelectBlock(blockId);
+                    blockIdChanged(blockId);


There is already a redux event for this, so prefer loose coupling and listen to that event instead.

xitij2000 · 2018-05-22T12:46:32Z

lms/djangoapps/instructor/static/instructor/ProblemReport/data/reducers/index.js

+
+export default combineReducers({
+    problemReport,
+});


This reducer is doing nothing.

xitij2000 · 2018-05-22T12:53:48Z

lms/djangoapps/instructor/static/instructor/ReportPopup/data/reducers/index.js

+                inProgress: action.inProgress,
+                succeeded: action.succeeded,
+                reportPath: action.reportPath
+            };


The success action here will not clear out the error message, you should add error: null or similar here.

xitij2000 · 2018-05-22T12:58:50Z

lms/djangoapps/instructor/static/instructor/ReportPopup/data/reducers/index.js

+                reportPath: action.reportPath
+            };
+        case problemResponsesPopupActions.ERROR:
+            return {...state, error: action.error};


Like the above, this should clear out succeeded.

xitij2000 · 2018-05-22T13:04:23Z

...tor/static/instructor/ProblemReport/components/ProblemReportWrapper/ProblemReportWrapper.jsx

+    courseId: PropTypes.string.isRequired,
+    excludeBlockTypes: PropTypes.arrayOf(PropTypes.string).isRequired,
+    endpoint: PropTypes.string.isRequired
+};


This isn't needed if you listen to the redux event.

xitij2000 · 2018-05-22T13:09:11Z

lms/djangoapps/instructor/static/instructor/ReportPopup/components/Popup/PopupModal.jsx

+        } else if (this.props.message != null) {
+            msg = (
+                <div
+                    className={`msg ${this.props.succeeded ? "msg-success" : "msg-warning"}`}>{this.props.message}</div>


Use the classnames library here.

xitij2000 · 2018-05-22T13:09:41Z

lms/djangoapps/instructor/static/instructor/ReportPopup/components/Popup/PopupModal.jsx

+                {msg}
+                <p>
+                    Once it's ready, you can view or download it using the buttons below. You can also close this popup
+                    now, and download the report later, from the "Reports Available for Download" area below.


This needs to be translatable as well.

xitij2000 · 2018-05-22T13:13:41Z

lms/djangoapps/instructor/static/instructor/ReportPopup/components/Popup/PopupModal.jsx

+        window.open(`/media/${this.props.reportPath}`, '_blank');
+    }
+
+    render() {


You can break this method down into getButtons, getBody, getProgress etc.

xitij2000 · 2018-05-22T13:20:50Z

lms/djangoapps/instructor/static/instructor/ReportPopup/data/api/client.js

+        credentials: 'same-origin',
+        method: 'get',
+        headers: HEADERS
+    });


Don't hardcode this URL either, pass it in like you did with "endpoint".

xitij2000 · 2018-05-22T13:26:06Z

lms/djangoapps/instructor/static/instructor/ReportPopup/components/Popup/PopupModal.jsx

+    inProgress: PropTypes.bool.isRequired,
+    succeeded: PropTypes.bool.isRequired,
+    reportPath: PropTypes.string,
+};


This can be a static functional component.

mduboseedx · 2018-05-22T13:33:35Z

@edx/educator-product Could you take a look at the UI and language for this PR? Thanks

xitij2000 · 2018-05-22T13:38:09Z

lms/djangoapps/instructor/static/instructor/ReportPopup/data/reducers/index.js

+
+export default combineReducers({
+    task,
+});


You don't need this since there is a single reducer :-)

xitij2000 · 2018-06-12T21:37:38Z

jenkins run all

bradenmacdonald · 2018-07-07T20:32:17Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Popup/PopupModal.jsx

+                }/>
+    );
+
+    const buttons = (reportPath === null && !error)


Shouldn't this be (reportPath !== null && !error) ? i.e. Doesn't reportPath get set once the report is available?

Yup, sorry. I think I made an amended commit for this but forgot to push it.

Co-authored-by: Adam Kovari <adam@opencraft.com> Co-authored-by: Eugeny Kolpakov <eugeny.kolpakov@gmail.com> Co-authored-by: Kshitij Sobti <kshitij@sobti.in>

xitij2000 · 2018-09-13T07:30:51Z

jenkins run bokchoy

edx-status-bot · 2018-09-13T08:08:06Z

Your PR has finished running tests. The following contexts failed:

jenkins/js
jenkins/bokchoy

xitij2000 · 2018-11-02T18:19:18Z

@mduboseedx Just checking in to see if this could get a rough review of the UI and language (as you suggested above) so we can make any adjustments.

mduboseedx · 2018-11-02T19:32:00Z

@xitij2000 I'm checking with product. Sorry for the delay here.

marcotuts · 2018-11-08T18:24:43Z

Hello @xitij2000

Can I ask why this is implemented as a modal / pop-up instead of improving the task list / queue currently rendered at the bottom of the reports / certificates /emails page?

1.) My preference would be to make improvements to this management view for asynchronous reports / requests so that then the value would be rippled across multiple places. I'm not comfortable initially incorporating this modal / blocking approach for conveying report status.
2.) Another option would be to seek to incorporate improvements to edx-ace to build in a class of signals / notifications for reports that would eventually be displayed in a notifications tray. This is preferable for asynchronous actions / events, even more so than option #1, but it is also perhaps much further into the future than what is possible incrementally. today.

Look forward to your thoughts on this. In its current form, I don't think this is something we'd like to merge into the core experience.

bradenmacdonald · 2018-11-08T19:01:21Z

@marcotuts Really quickly: the list at the bottom often has dozens and dozens of reports with long and confusing names (and small font), and it may include reports requested by other users. So it can be hard and slow to find the one you want in the list. By making this a popup that only shows the report you just requested, there's no work required to hunt and find your report in the list, check that it's ready, then download it. It's right in your face.

I think this makes it far, far easier to use from a user's perspective. I expect users would leave this open in a spare tab on their browser until they see that it's complete, so it wouldn't be "blocking" work anymore than leaving the report page open and continuously reviewing the list of reports would be.

Option 2 sounds great, but we don't have any budget to make either of those other things happen at this point.

marcotuts · 2018-11-09T03:13:46Z

@bradenmacdonald - Is there a path to making that first report in the bottom grouped / rendered differently to show you "your reports" by default and have a toggle to view "all report"?

We could make that area far more useful for all users without hopefully introducing another modal UI pattern for the reports workflow. I can try to suggest / how we might visually bridge the gap between the new modal you are proposing and how that simplified view might look if incorporated into an updated "Reports Queue" area that improves on the current one.

Totally understood that option 2 isn't reasonable necessarily within the scope here.

xitij2000 · 2018-11-11T09:30:17Z

@marcotuts As @bradenmacdonald mentioned above, while this is a modal, it doesn't really block users from using another tab, or even just navigating away. Perhaps that is something that can be made even clearer in the language on the modal? Currently I think it's pretty clear that you can dismiss the popup and find the report later. Or perhaps the popup can not darken the background?

I don't think the user who requested the report is currently tracked, so a your-reports vs all-reports toggle would need a bigger change than is in scope here.

marcotuts · 2018-11-26T16:40:34Z

As proposed I'm not comfortable adding this modal on top of the current patterns / views. I'm marking this closed and we can revisit other cleanups to this hopefully soon, but it sounds like not in scope for the current effort.

bradenmacdonald · 2018-11-26T17:43:02Z

@marcotuts We had shown that modal to Shelby in March and she was fine with it, which is why we implemented it as shown. Hopefully the meetings and other process changes you've discussed with me and Gabriel can help avoid these sort of situations :/ Since at this point, we won't have any budget to follow up on this or re-implement it: is this report generation workflow UI something edX might be able to improve before the next named release?

xitij2000 · 2019-01-18T11:49:12Z

@marcotuts Sorry for the delay here, but I had an alternative proposal for the UI, which would be more inline rather than a popup. It would work as follows:

User clicks on generate report button
There is a spinner next to the button with text like "report is being generated"
When the report is generated, the spinner becomes a checkmark, and the text is replaced with a link to the report
The link just takes you to the reports list at the bottom of the page but highlights the report you wanted.
In case of an error, the error can also be displayed inline next to the generate report button.

Would a UI like the above fit in better?

marcotuts · 2019-02-13T01:30:07Z

Ah, apologies for missing this. If we could avoid the use of the modal interrupt but provide improved visibility into the report's download progress that would be great. At a high level your proposal for changes @xitij2000 sounds great, and if that isn't too much effort to change that would be great! Let us know when this would be ready for engineering review, or if after review the changes you described prove to be much larger than expected. Thanks!

marcotuts · 2019-02-13T01:30:38Z

Ah I can't reopen this branch as it was deleted, it looks like this would need to be separately reopened - @xitij2000 .

xitij2000 · 2019-02-14T11:13:47Z

I will create a new PR for this with the approach I outlined above.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 22, 2018

xitij2000 reviewed May 22, 2018

View reviewed changes

openedx-webhooks added product review PR requires product review before merging and removed needs triage labels May 22, 2018

xitij2000 reviewed May 22, 2018

View reviewed changes

akovari force-pushed the adam/instructor_report_popup branch from 4e1bae4 to 3a5bf36 Compare May 30, 2018 18:11

xitij2000 force-pushed the adam/instructor_report_popup branch 3 times, most recently from e63420e to 4dd91f0 Compare June 14, 2018 03:54

xitij2000 force-pushed the adam/instructor_report_popup branch from 4dd91f0 to dcaa4ac Compare July 5, 2018 11:25

bradenmacdonald reviewed Jul 7, 2018

View reviewed changes

xitij2000 force-pushed the adam/instructor_report_popup branch from dcaa4ac to 900d73d Compare July 8, 2018 14:23

xitij2000 force-pushed the adam/instructor_report_popup branch 4 times, most recently from 1c09600 to 65fcf58 Compare July 17, 2018 21:26

Add a CSV Viewer for viewing CSV reports in the browser.

ca0d59d

xitij2000 force-pushed the adam/instructor_report_popup branch from 65fcf58 to f65bc4a Compare September 12, 2018 13:15

Add popup to display report creation progress.

b6dae78

Co-authored-by: Adam Kovari <adam@opencraft.com> Co-authored-by: Eugeny Kolpakov <eugeny.kolpakov@gmail.com> Co-authored-by: Kshitij Sobti <kshitij@sobti.in>

xitij2000 force-pushed the adam/instructor_report_popup branch from f65bc4a to b6dae78 Compare September 12, 2018 13:56

marcotuts closed this Nov 26, 2018

pomegranited deleted the adam/instructor_report_popup branch January 14, 2019 05:21

xitij2000 mentioned this pull request Mar 13, 2019

Inline problem response report status #19986

Merged

1 task

openedx-webhooks added rejected and removed product review PR requires product review before merging labels Jan 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XBlock Reporting: Display a popup after requesting a report #18244

XBlock Reporting: Display a popup after requesting a report #18244

akovari commented May 22, 2018

openedx-webhooks commented May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

xitij2000 May 22, 2018

akovari May 25, 2018

mduboseedx commented May 22, 2018

xitij2000 May 22, 2018

xitij2000 commented Jun 12, 2018

bradenmacdonald Jul 7, 2018

xitij2000 Jul 7, 2018

xitij2000 commented Sep 13, 2018

edx-status-bot commented Sep 13, 2018

xitij2000 commented Nov 2, 2018

mduboseedx commented Nov 2, 2018

marcotuts commented Nov 8, 2018

bradenmacdonald commented Nov 8, 2018 •

edited

Loading

marcotuts commented Nov 9, 2018

xitij2000 commented Nov 11, 2018

marcotuts commented Nov 26, 2018

bradenmacdonald commented Nov 26, 2018

xitij2000 commented Jan 18, 2019

marcotuts commented Feb 13, 2019

marcotuts commented Feb 13, 2019

xitij2000 commented Feb 14, 2019

XBlock Reporting: Display a popup after requesting a report #18244

XBlock Reporting: Display a popup after requesting a report #18244

Conversation

akovari commented May 22, 2018

openedx-webhooks commented May 22, 2018

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

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

mduboseedx commented May 22, 2018

Choose a reason for hiding this comment

xitij2000 commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 commented Sep 13, 2018

edx-status-bot commented Sep 13, 2018

xitij2000 commented Nov 2, 2018

mduboseedx commented Nov 2, 2018

marcotuts commented Nov 8, 2018

bradenmacdonald commented Nov 8, 2018 • edited Loading

marcotuts commented Nov 9, 2018

xitij2000 commented Nov 11, 2018

marcotuts commented Nov 26, 2018

bradenmacdonald commented Nov 26, 2018

xitij2000 commented Jan 18, 2019

marcotuts commented Feb 13, 2019

marcotuts commented Feb 13, 2019

xitij2000 commented Feb 14, 2019

bradenmacdonald commented Nov 8, 2018 •

edited

Loading