Skip to content

Conversation

@pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Nov 15, 2021

Description

Builds on the shim added by https://github.com/edx/edx-platform/pull/29190 to deprecate the ModuleSystem.xqueue property, which is only used by CAPA problems.

This change is only a refactoring, and should not affect Learners, Course Authors, or anyone else using edx-platform.

Supporting information

Testing instructions

LMS:

  1. Login as a demo learner (honor@example.com / edx)

  2. Enroll in the BD-13 test course

  3. Navigate to Complex blocks > External grader

  4. Ensure that you can submit answers to the externally graded problem.

    ⚠️ Note: the grader is dumb, and so always returns "correct", so it doesn't matter what you submit.

  5. Repeat this test in the legacy course experience.

Studio:

  1. Login as a demo staff (staff@example.com / edx)

  2. Navigate to the BD-13 test course > Complex blocks > External grader

  3. Try submitting answers to the problem.

    You'll get a message that reads "Error: No grader has been set up for this problem. " because externally graded problems can only be graded in preview / LMS mode.

    You can verify this behaviour on the sandbox created for https://github.com/edx/edx-platform/pull/29260.

  4. View the problem in Preview mode, and ensure you can submit answers to the problem as you can in the LMS.

Regression testing:

LMS:

  1. Login as a demo learner (honor@example.com / edx)
  2. Browse around the Demo course, paying particular attention to the CAPA problems.
  3. Check that you can submit answers to problems, and save changes to blocks.
  4. Switch to the Legacy Course Experience and ensure you can submit answers to problems there too.

Studio:

  1. Login as a demo staff (staff@example.com / edx)
  2. Navigate to the Demo course and browse through the blocks, paying particular attention to the CAPA problem blocks. Ensure they render and edit as expected.
  3. Check that you can submit answers to problems, and save changes to blocks.
  4. Preview unpublished changes and ensure they render as expected.
  5. Publish changes, and ensure they render in the LMS as expected.

Deadline

None

Author Notes & Concerns

  1. See this comment for question about xqueue_callback_url_prefix.

Notes for hacking setting up the dummy external grader:

# on the sandbox appserver:
sudo mkdir /edx/app/grader
sudo chown pomegranited: /edx/app/grader
cd /edx/app/grader

# set up virtualenv
sudo apt install python3.8-venv
python3 -m venv venv/grader

# check out my dummy grader branch
git clone https://github.com/open-craft/xqueue-watcher/
cd xqueue-watcher
git checkout jill/bd-13-dummy-grader

# Run the grader in a screen session
screen -S grader
source /edx/app/grader/venv/grader/bin/activate
make requirements  # had to run this twice to get around some error with building a wheel for dogstatsd-python
python -m xqueue_watcher -d .
# suspend screen session

@openedx-webhooks
Copy link

openedx-webhooks commented Nov 15, 2021

Thanks for the pull request, @pomegranited! I've created BLENDED-1013 to keep track of it in Jira. More details are on the BD-13 project page.

When this pull request is ready, tag your edX technical lead.

@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Nov 15, 2021
@pomegranited pomegranited marked this pull request as draft November 15, 2021 05:16
@pomegranited pomegranited force-pushed the jill/BD-13-xqueue branch 2 times, most recently from 22d35fc to 835fa0f Compare November 15, 2021 09:38
@pomegranited pomegranited marked this pull request as ready for review November 16, 2021 07:44
@pomegranited
Copy link
Contributor Author

jenkins run python

Copy link
Contributor

@symbolist symbolist left a comment

Choose a reason for hiding this comment

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

Please rebase and squash as necessary and then this will be ready to ship! 👍🏽

  • I tested this according to the test instructions.
  • I read through the code
  • I checked for accessibility issues. N/A since no UI changes.
  • Includes documentation

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: followed the testing instructions
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

* Deprecates ModuleSystem.xqueue property
* Adds new XQueueService to provide the deprecated property values to the LMS runtime
  (Studio does not need the XQueueService.)
* Adds tests for new service and updates the ModuleSystemShim tests in LMS and Studio
* Fixes existing tests.
* Uses the XQueueService object from the ModuleSystem in the LoncapaSystem
  instead of the dict of xqueue-related values.
* Adds a StubXQueueService for testing.
from the lms get_module_for_descriptor and related methods.

This prefix was previously inferred from the request's base URL and
passed through several methods to reach the XQueueService creation.
But the request base URL always matches the LMS_ROOT_URL (or the preview
URL), so this change simply uses that setting instead.
@edx-status-bot
Copy link

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

@Agrendalath
Copy link
Member

@jristau1984, I'm merging this, as discussed on Slack.

@Agrendalath Agrendalath merged commit 2ef69d5 into openedx:master Dec 20, 2021
@Agrendalath Agrendalath deleted the jill/BD-13-xqueue branch December 20, 2021 14:41
@openedx-webhooks
Copy link

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

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

Labels

blended PR is managed through 2U's blended developmnt program merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants