Skip to content

Fixing any_unfulfilled_milestones in enrollments API - #14758

Closed
iamjazzar wants to merge 2 commits into
openedx:masterfrom
iamjazzar:jazzar/fix-enrollments-prerequisites
Closed

Fixing any_unfulfilled_milestones in enrollments API#14758
iamjazzar wants to merge 2 commits into
openedx:masterfrom
iamjazzar:jazzar/fix-enrollments-prerequisites

Conversation

@iamjazzar

@iamjazzar iamjazzar commented Mar 23, 2017

Copy link
Copy Markdown
Contributor

Description

In User Course Enrollments List: /api/mobile/v0.5/users/username/course_enrollments, if we enabled Entrance Exam from Schedule & Details section and then re-desabling it, thecourseware_access field in the API will keep preventing the student from showing the course on the mobile apps.

            "courseware_access": {
                "has_access": false,
                "error_code": "unfulfilled_milestones",
                "developer_message": "User has unfulfilled milestones",
                "user_message": "User has unfulfilled milestones."
            },

This problem is happening because the get_course_milestones_fulfillment_paths method in the milestones API is returning an empty dictionary {} if the course doesn't have any milestones, after enabling Entrance Exam it returns a complex dict-of-dicts like this:

{
  "course-v1:org+course+run.entrance_exams.Completed Course Entrance Exam": {
     "content": [
          "block-v1:org+course+run+type@chapter+block@cd5f1b8bef874bcdad80b2857f5aeffd"
     ]
  }
}

While redoubling the entrance exam causes the method to return the previously-enabled prerequisites with an empty dictionary like this:

{"course-v1:org+course+run.entrance_exams.Completed Course Entrance Exam": {}}

The method any_unfulfilled_milestones is not addressing this scenario, so what I did is checking that every value in the dict returned from get_course_milestones_fulfillment_paths, if any value has a non-empty dictionary then the student is still having unfulfilled milestones and we are returning True, if the dictionary is empty or the student have fulfilled the milestones then we are returning False.

Note

This solution won't affect the result if the learner completed the prerequisites since the get_course_milestones_fulfillment_paths will return an empty dictionary if the Entrance Exam fulfilled.

TODO

  • Squash and rebase after review.

Update

Another issue arised as if the instructor enabled and disabled the Prerequisite Course in Studio's Requirements section, the course became inaccessible for learner in the dashboard and the API views.
This happens because selecting None from the dropdown list in Studio has no functionality in the code.

@openedx-webhooks

Copy link
Copy Markdown

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

@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-prerequisites branch 2 times, most recently from 542144e to ccd3763 Compare March 24, 2017 14:36
@gsong

gsong commented Apr 19, 2017

Copy link
Copy Markdown
Contributor

Jenkins ok to test

@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-prerequisites branch 2 times, most recently from 6efb4b7 to a1ee777 Compare April 20, 2017 10:58
@iamjazzar

Copy link
Copy Markdown
Contributor Author

Tests are passing now after the rebase.

@iamjazzar

Copy link
Copy Markdown
Contributor Author

The branch updated with another commit.

@OmarIthawi

Copy link
Copy Markdown
Member

Hey @gsong I've updated the branch again to fix a recent merge conflict. Please let me know if I need anything else to do.

@OmarIthawi

Copy link
Copy Markdown
Member

Hey @gsong, just a gentle reminder about this PR.

@OmarIthawi
OmarIthawi force-pushed the jazzar/fix-enrollments-prerequisites branch from ddcb1af to f8c80b6 Compare June 21, 2017 03:01
@gsong
gsong requested review from BenjiLee and removed request for BenjiLee June 21, 2017 16:42
@gsong

gsong commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

@andy-armstrong @BenjiLee Do you know who might be the right person to review this PR?

@BenjiLee

Copy link
Copy Markdown
Contributor

@gsong Probably me. @mattdrayer would be another option.

@gsong

gsong commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

@BenjiLee Would appreciate getting this into a sprint, let me know the best way to do that. Thank you.

@BenjiLee

Copy link
Copy Markdown
Contributor

@jasperho CC

@OmarIthawi

Copy link
Copy Markdown
Member

Thanks a lot @BenjiLee and @gsong 😃

@gsong

gsong commented Jun 26, 2017

Copy link
Copy Markdown
Contributor

jenkins run all

@gsong

gsong commented Jun 26, 2017

Copy link
Copy Markdown
Contributor

Jenkins ok to test

@OmarIthawi
OmarIthawi force-pushed the jazzar/fix-enrollments-prerequisites branch from f8c80b6 to 260225e Compare June 27, 2017 11:15
@OmarIthawi

Copy link
Copy Markdown
Member

Thanks @gsong, I'll fix the brokens tests/quality checks.

@OmarIthawi
OmarIthawi force-pushed the jazzar/fix-enrollments-prerequisites branch from 260225e to 43437fb Compare June 27, 2017 23:40
@OmarIthawi

Copy link
Copy Markdown
Member

@AhmedAljazzar I've rebase this PR and fixed both of the conflicts and PEP8 issue. Unfortunately, I left the subtle broken test to you. Please check jenkins/python suite.

@iamjazzar

Copy link
Copy Markdown
Contributor Author

@OmarIthawi looks like we lost remove_prerequisite_course import during the rebase. I imported it again and waiting for tests results.

@OmarIthawi

Copy link
Copy Markdown
Member

Thanks @AhmedAljazzar, looks like the tests are good. CC: @gsong

@gsong

gsong commented Jul 10, 2017

Copy link
Copy Markdown
Contributor

@jasperho Can you help us schedule this into a sprint review? Thanks.

@OmarIthawi

Copy link
Copy Markdown
Member

Just a gentle reminder @jasperho.

@BenjiLee

Copy link
Copy Markdown
Contributor

This ticket is currently in our sprint and will be reviewed.

@OmarIthawi

Copy link
Copy Markdown
Member

Thanks @BenjiLee! Looking forward for your feedback.

@OmarIthawi

Copy link
Copy Markdown
Member

@AhmedAljazzar I think this one bug deserves a test case to ensure it survives future updates. Let's work on it today if you have time.

@OmarIthawi
OmarIthawi force-pushed the jazzar/fix-enrollments-prerequisites branch from f6761a1 to 45f607f Compare July 19, 2017 09:33
@OmarIthawi
OmarIthawi force-pushed the jazzar/fix-enrollments-prerequisites branch from 45f607f to 5ddb1dd Compare July 19, 2017 09:34
@BenjiLee

Copy link
Copy Markdown
Contributor

This LGTM. Waiting on testcase.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Jul 19, 2017
@OmarIthawi

Copy link
Copy Markdown
Member

Thanks @BenjiLee! Could you please mark this as ok to test?

@OmarIthawi

Copy link
Copy Markdown
Member

I did some refactoring as well. Please review and let me know your feedback.

@OmarIthawi

Copy link
Copy Markdown
Member

@AhmedAljazzar could you please do a quick review? I'd like to squash the commits.

@OmarIthawi
OmarIthawi force-pushed the jazzar/fix-enrollments-prerequisites branch from a19792f to bc64e52 Compare July 20, 2017 10:45
@gsong

gsong commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

Jenkins ok to test

@gsong

gsong commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

jenkins run all

@OmarIthawi

OmarIthawi commented Jul 20, 2017

Copy link
Copy Markdown
Member

@gsong how about sudo jenkins run all 😄

@gsong

gsong commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

@OmarIthawi Jenkins is a bit backed up right now, it'll get to this eventually.

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

Waiting on passing tests

@gsong gsong mentioned this pull request Jul 20, 2017
@gsong

gsong commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

Merged via #15623.

@gsong gsong closed this Jul 20, 2017
@gsong gsong removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 20, 2017
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants