Skip to content

[OC-4409] Browser-Based CSV Viewer for Reports - #18231

Closed
xitij2000 wants to merge 1 commit into
openedx:masterfrom
open-craft:kshitij/csv-viewer
Closed

[OC-4409] Browser-Based CSV Viewer for Reports#18231
xitij2000 wants to merge 1 commit into
openedx:masterfrom
open-craft:kshitij/csv-viewer

Conversation

@xitij2000

@xitij2000 xitij2000 commented May 18, 2018

Copy link
Copy Markdown
Contributor

Adds a purely static CSV viewer to allow viewing CSV reports in the browser. Also adds a link to preview reports generated in the data download page of the instructor dashboard.

Project details:
This PR is part of the "Improved Problem Response Report" project, funded by Australian National University (ANU) and developed by OpenCraft

JIRA tickets: OSPR-2424 / OC-4409

Discussions: https://edxchange.opencraft.com/t/xblock-reporting-tool/191

Screenshots:
Live preview link:
https://pr18231.sandbox.opencraft.hosting/csv-viewer?csvUrl=https://data.cityofnewyork.us/api/views/25th-nujf/rows.csv?accessType=DOWNLOAD
screen shot 2018-05-22 at 19 25 37

Sandbox URL:

Merge deadline: "None"

Testing instructions:

  1. Visit /csv-viewer?csvUrl=URL_TO_CSV_FILE
  2. You should see a sortable, filterable table that displays the contents of the CSV file for viewing in the browser.
    OR.
  3. Visit the download page in the instructor dashboard.
  4. Generate a report.
  5. Click on the "Preview" button in the task list.
  6. The report should open in the browser for preview.

Example url: https://pr18231.sandbox.opencraft.hosting/csv-viewer?csvUrl=https://data.cityofnewyork.us/api/views/25th-nujf/rows.csv?accessType=DOWNLOAD

Author notes and concerns:

To view reports stored on S3, SWIFT or some other remote storage, it will need to have CORS set up.

Reviewers

@openedx-webhooks

Copy link
Copy Markdown

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

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 18, 2018
@xitij2000 xitij2000 changed the title [WIP]: CSV Report Viewer WIP: CSV Report Viewer May 18, 2018
@xitij2000
xitij2000 force-pushed the kshitij/csv-viewer branch from 32f0ee0 to 57e1b61 Compare May 18, 2018 10:12

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems papaparse is able to download remote files by itself, maybe that simplify things here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure will do, that does seem cleaner.

@mduboseedx

Copy link
Copy Markdown
Contributor

@xitij2000 Let me know when this is ready for review. Thanks!

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels May 18, 2018
@xitij2000
xitij2000 force-pushed the kshitij/csv-viewer branch 4 times, most recently from 33fd0ac to 6b5e77a Compare May 23, 2018 12:51
@xitij2000

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

@xitij2000 xitij2000 changed the title WIP: CSV Report Viewer Browser-Based CSV Viewer for Reports May 23, 2018

@viadanna viadanna left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good to go! 👍

  • I tested this: ran the unit tests, tested in the devstack
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@xitij2000

Copy link
Copy Markdown
Contributor Author

@mduboseedx This is ready for review now.

@mduboseedx

Copy link
Copy Markdown
Contributor

@edx/educator-product This PR adds a CSV viewer from within the browser. Could you give this a review and confirm it looks ok from your perspective?

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 24, 2018
@shamck

shamck commented Jun 13, 2018

Copy link
Copy Markdown

This looks good from a product perspective. I'm updating the JIRA ticket to "Awaiting prioritization".

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed product review PR requires product review before merging labels Jun 13, 2018
@bradenmacdonald

Copy link
Copy Markdown
Contributor

Thanks @shamck !

@xitij2000
xitij2000 force-pushed the kshitij/csv-viewer branch 3 times, most recently from 4cd4b21 to 8b938e1 Compare July 18, 2018 09:39
@xitij2000
xitij2000 force-pushed the kshitij/csv-viewer branch from 4cfc214 to 281fd5d Compare July 29, 2018 09:32
@xitij2000

Copy link
Copy Markdown
Contributor Author

@mduboseedx Could you help get this PR reviewed? It's been waiting for some time :-)

@mduboseedx

Copy link
Copy Markdown
Contributor

@xitij2000 Sorry for the delay here and I understand that it has been waiting for a long time. Unfortunately I've learned the Educator team that would normally review this code, will actually be unable to for another few weeks. I'll see if another team or person could fill in for a review in the meantime. How does this PR fit in with your priorities?

@xitij2000

Copy link
Copy Markdown
Contributor Author

@marcotuts Would love to get an update on this as well, it's been in limbo for a while.

@natabene

Copy link
Copy Markdown
Contributor

@xitij2000 I am reaching out to Marco as well, will update you soon. Sorry for delay.

@wittjeff

Copy link
Copy Markdown

Accessibility-wise, some comments:

  • First, AG-Grid is a great place to start for accessible complex grids, since they have done a lot of heavy lifting already.
    Still I see a few issues in the demo implementation:
  • The user can't tab into the grid (move focus to the grid via tab key press, then navigate within via arrow key presses). The ability to do so was added with v15, so I wonder if this is a configuration thing.
  • The column sorting isn't exposed to screen reader users. I think this could be done with some minor changes to the current logic -- add relevant aria-labels for the spans that insert the up and down arrow symbols. For WCAG 2.1 conformance I think you might need to use aria-sort attributes instead.
  • The sorting / column header menus aren't keyboard-accessible, and they don't use regular menu semantics. I'd bet you could implement your own custom alternative within the column headers fairly easily though.

@xitij2000

Copy link
Copy Markdown
Contributor Author

@wittjeff Thanks for looking into this! Since this PR was created AG-Grid has now reached v20, so I will give it a spin and see if that solves some of these issues, and otherwise will try to look into the workarounds you mentioned.

@marcotuts

Copy link
Copy Markdown
Contributor

Hello!

We can provide some feedback on options for making this a plugin, or a separate micro front end, but for the time being I do not think as it currently stands this should be merged into edx-platform. I will connect with @nasthagiri for her guidance on this, and we can perhaps discuss soon in person #OpenedX #SanDiego for options that are reasonable in scope to get this improvement landed somehow. Thanks!

@marcotuts marcotuts closed this Feb 8, 2019
@openedx-webhooks

Copy link
Copy Markdown

@xitij2000 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@marcotuts @nasthagiri AFAIK the UI integration points that we'd need to make this a plugin (on the reports page in the instructor dashboard) don't exist, or we would have done it that way. We're already over-budget on this project, so can't spend the time for a big refactor. If you can add the requisite plugin API / UI integration point, then we can make this a plugin, sure.

@marcotuts marcotuts reopened this Feb 13, 2019
@marcotuts

Copy link
Copy Markdown
Contributor

After some discussion, it looks like I missed an earlier approval for this change from the edX standpoint before we had started shifting toward plugin contributions & the goal of a separate edX instructor product / front-ends outside of edx-platform. While we would still prefer this future, we don't have a base instructor-dashboard-front-end implementation to steer a contribution like this towards. While our future goals aren't changed, this is a long standing PR that we would like to now move through to engineering prioritization. I will work with edX teams to see how we can get this reviewed. Thanks everyone!

@edx-status-bot

Copy link
Copy Markdown

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

  • jenkins/python
  • jenkins/lettuce
  • jenkins/bokchoy
  • jenkins/quality
  • codecov/project

@xitij2000

Copy link
Copy Markdown
Contributor Author

@marcotuts Did you get a chance to discuss how to handle this PR?

@marcotuts

Copy link
Copy Markdown
Contributor

Apologies for the delay and missing this @xitij2000. This can move to engineering review next, I will connect with @ormsbee and others to determine next steps. FYI @natabene

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed product review PR requires product review before merging labels May 31, 2019
@xitij2000

Copy link
Copy Markdown
Contributor Author

Thanks!

@xitij2000

Copy link
Copy Markdown
Contributor Author

@ormsbee @marcotuts
I've been working on making this an external plugin, which is simple enough, however we need a way to integrate with the platform itself so that CSV reports can be opened using the online browser. Since there is no clean API to do this, I've submitted a PR that allows plugins to inject code into the templates just as one would with a theme, and using the same extension points.

The goal then will be to inject some simple JavaScript code that will cause all .csv links to open in this web-based CSV viewer.

We have other use-cases for this as well which are currently less defined. I would love to get feedback on this and see how we can get that PR into the platform so we can close this PR and move it to an external plugin.

@xitij2000 xitij2000 closed this Dec 12, 2019
@openedx-webhooks

Copy link
Copy Markdown

@xitij2000 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@xitij2000

Copy link
Copy Markdown
Contributor Author

Closed in favour of plugin-based approach using API from https://github.com/edx/edx-platform/pull/21968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants