Skip to content
This repository was archived by the owner on Jul 28, 2020. It is now read-only.

Fixing any_unfulfilled_milestones in mobile course_enrollments API - #280

Merged
OmarIthawi merged 2 commits into
masterfrom
jazzar/fix-enrollments-api-prerequisites
May 30, 2017
Merged

Fixing any_unfulfilled_milestones in mobile course_enrollments API#280
OmarIthawi merged 2 commits into
masterfrom
jazzar/fix-enrollments-api-prerequisites

Conversation

@iamjazzar

@iamjazzar iamjazzar commented Mar 23, 2017

Copy link
Copy Markdown
Contributor

Description

In User Course Enrollments List, 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.

Notes

  1. 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.
  2. EdX PR: #14758

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.

@iamjazzar iamjazzar changed the title Fixing any_unfulfilled_milestones in enrollments API Fixing any_unfulfilled_milestones in mobile course_enrollments API Mar 23, 2017
@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-api-prerequisites branch from 509b9e5 to b37e736 Compare March 24, 2017 12:15

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

I think this calls for an OSPR, please contribute it to edX and cherry pick


from milestones import api as milestones_api
fulfillment_paths = milestones_api.get_course_milestones_fulfillment_paths(course_id, {'id': user_id})

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.

Just one comment, why not use the function in this file: get_course_milestones_fulfillment_paths ?
I see in some functions the milestones_api.get_course_..... is being used, and only in one function the get_course_milestones_fulfillment_paths in the file is being used. I do not see the difference in the result on one hand, and I do not see why from milestones import api as milestones_api has to be used many times?

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.

The other methods are using the API gateway, so I prefer keeping it like this. In the same hand, the get_course_milestones_fulfillment_paths function is rechecking the feature flag again, a reason that tells not call it.

Let's wait for edX's review and see. But we still need to know the status of the changes if we need had to merge this before edX do.

@Salomari1987

Copy link
Copy Markdown
Contributor

@AhmedAljazzar ping

@iamjazzar

Copy link
Copy Markdown
Contributor Author

@Salomari1987 pong.

@Salomari1987

Copy link
Copy Markdown
Contributor

@AhmedAljazzar this works fine as expected, however when tried for course prerequisites, the has_access field will become false for the nonsuperuser. Please fix it also for course prerequisites.

@Salomari1987

Copy link
Copy Markdown
Contributor

and rebase

@Salomari1987 Salomari1987 changed the title Fixing any_unfulfilled_milestones in mobile course_enrollments API [WIP] Fixing any_unfulfilled_milestones in mobile course_enrollments API May 21, 2017
@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-api-prerequisites branch from b37e736 to 7b668bf Compare May 29, 2017 11:50
@iamjazzar

Copy link
Copy Markdown
Contributor Author

@Salomari1987 Done :)

@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-api-prerequisites branch 2 times, most recently from 0641d0e to b84b304 Compare May 29, 2017 12:10
@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-api-prerequisites branch from b84b304 to f67df39 Compare May 29, 2017 12:13
@iamjazzar iamjazzar changed the title [WIP] Fixing any_unfulfilled_milestones in mobile course_enrollments API Fixing any_unfulfilled_milestones in mobile course_enrollments API May 29, 2017
@iamjazzar
iamjazzar force-pushed the jazzar/fix-enrollments-api-prerequisites branch from 857ceca to f67df39 Compare May 29, 2017 12:37
@iamjazzar

Copy link
Copy Markdown
Contributor Author

@Salomari1987, @halawa : This needs approval if you have time.

@OmarIthawi

Copy link
Copy Markdown
Contributor

I'll take it!

@iamjazzar

Copy link
Copy Markdown
Contributor Author

Great!

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

Smoke tested it and it works. The code looks fine although I haven't tested it rigorously.

@OmarIthawi
OmarIthawi merged commit d0f04dc into master May 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants