Skip to content

Conversation

@xitij2000
Copy link
Contributor

@xitij2000 xitij2000 commented Aug 28, 2020

In a previous PR (https://github.com/edx/edx-platform/pull/19986) to implement a report status UI, a change was made that added the report path to the task output. This was to simplify getting this data during the task progress, so we could update the UI with it. However, this change makes the task output too large which causes it to fail in the last step in many cases.

This PR updates the code to use alternative method for getting report download link using the report downloads list.

JIRA tickets: OSPR-4931

Discussions: https://github.com/edx/edx-platform/pull/19986#issuecomment-682377341

Dependencies: None

Sandbox URL:
LMS: https://pr24867.sandbox.opencraft.hosting/
Studio: https://studio.pr24867.sandbox.opencraft.hosting/

Merge deadline: ASAP

Testing instructions:

  1. Visit the instructor dashboard.
  2. Generate a problem responses report
  3. Check that browser console for requests to the instructor_task_status endpoint
  4. Without applying this PR, they should include a report_path parameter
  5. After applying this PR that parameter is no longer present.

Reviewers

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

@openedx-webhooks
Copy link

Thanks for the pull request, @xitij2000! I've created OSPR-4931 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 28, 2020
@DawoudSheraz
Copy link
Contributor

@xitij2000 Hi. Just to catch-up, do we have an estimated timeline for this PR review and merge? Thanks

@xitij2000
Copy link
Contributor Author

@DawoudSheraz Unfortunately I will only find time to properly work on this next sprint. There are still some issues and it needs an internal review.

@natabene
Copy link
Contributor

natabene commented Sep 2, 2020

@xitij2000 Thank you for your contribution. Please let me know once it is ready for our review.

@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 Sep 2, 2020
@xitij2000 xitij2000 force-pushed the kshitij/remove-report-path branch 3 times, most recently from c5fbc1f to 64c2226 Compare September 8, 2020 14:09
@xitij2000 xitij2000 changed the title Remove report path from task output data [BB-2967] Remove report path from task output data Sep 8, 2020
@DawoudSheraz
Copy link
Contributor

@xitij2000 Changes look good. I have tested on sandbox too. Please let me know the expected merge timeline after your internal review.

@0x29a
Copy link
Contributor

0x29a commented Sep 9, 2020

@xitij2000, I noticed new error in console:
Screenshot from 2020-09-09 20-13-33

It's very minor, you can fix it by changing: https://github.com/edx/edx-platform/blob/64c2226a44246af165a8ef09fa9e136c056d5680/lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.jsx#L46

To this:

<input type="text" name="problem-location" value={selectedBlock || ''} disabled />

Copy link
Contributor

@0x29a 0x29a left a comment

Choose a reason for hiding this comment

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

👍 after fixing mentioned error.

  • I tested that requests to instructor_task_status don't contain report_path parameter
  • I read through the code
  • I checked for accessibility issues No new interfaces introduced.
  • Includes documentation No need in additional documentation here.
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@xitij2000
Copy link
Contributor Author

@xitij2000, I noticed new error in console:
Screenshot from 2020-09-09 20-13-33

It's very minor, you can fix it by changing:

https://github.com/edx/edx-platform/blob/64c2226a44246af165a8ef09fa9e136c056d5680/lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.jsx#L46

To this:

<input type="text" name="problem-location" value={selectedBlock || ''} disabled />

I've fixed it a different way. This is happening because the original value for selected block in null in the reducer. I've now set it to an empty string.

@xitij2000
Copy link
Contributor Author

xitij2000 commented Sep 10, 2020

@natabene This is ready for edX review.

CC: @DawoudSheraz

@natabene
Copy link
Contributor

@DawoudSheraz Can you review this and merge if you find it good?

@openedx-webhooks openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 10, 2020
@DawoudSheraz
Copy link
Contributor

@DawoudSheraz Can you review this and merge if you find it good?

Sure.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

@xitij2000 Will the related tests be added in a follow-up PR? Other than that, the changes look good to me.

@DawoudSheraz
Copy link
Contributor

@xitij2000 Also, can you rebase and squash. your commits? The tests seem to be stuck in getting the status back. I want to get this merged today if possible.

Use alternative method for getting report download link
@0x29a 0x29a force-pushed the kshitij/remove-report-path branch from db0fc9a to 9adeeb0 Compare September 11, 2020 12:12
@DawoudSheraz
Copy link
Contributor

jenkins run py38 quality

@edx-status-bot
Copy link

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

@DawoudSheraz DawoudSheraz merged commit 1d9c7e9 into openedx:master Sep 11, 2020
@openedx-webhooks
Copy link

@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.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

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

@xitij2000 xitij2000 deleted the kshitij/remove-report-path branch September 15, 2020 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants