Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: AA-1205: Add Learning MFE support for Entrance Exams #30001

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

Dillon-Dumesnil
Copy link
Contributor

@Dillon-Dumesnil Dillon-Dumesnil commented Mar 2, 2022

  • Adds entrance exam information to the Course Overview object
    • Enables hiding other tabs since the get_course_tab_list uses
      a Course Overview
    • Enables using the entrance exam helper functions to determine
      if Entrance exams are being used in this course.
  • Posts a message when Entrance Exam is passed to parent container for
    usage in the Learning MFE
  • Overrides the 'title' field of the courseware tab since the Learning MFE
    uses that over the 'name' field.

Related to openedx/frontend-app-learning#840

Copy link
Contributor

@mikix mikix left a comment

Choose a reason for hiding this comment

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

Nice.

lms/djangoapps/courseware/tabs.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/courseware_api/views.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/courseware_api/views.py Outdated Show resolved Hide resolved
@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/entrance-exam-support-aa-1205 branch from 7132011 to 3563a75 Compare March 4, 2022 15:41
entrance_exam_score_ratio = 0.0, 0.0
entrance_exam_score = 0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is a bit of a change, but honestly, this default return value is wild. I think it was meant to represent the earned, possible grade, but forgot to do the division. The output of this is never expecting a tuple and this is the only spot that would return it as such. So I updated it and renamed the function since it's just a score.

Comment on lines -71 to +68
version = IntegerField()
version = models.IntegerField()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these changes are just to be more inline with the preferred way of defining these fields according to django

@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/entrance-exam-support-aa-1205 branch 2 times, most recently from dd793a8 to e592c19 Compare March 4, 2022 15:56
name='entrance_exam_minimum_score_pct',
field=models.FloatField(default=0.65),
),
migrations.AddField(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what historicalcourseoverview is. What do these fields do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The historical model keeps a changelog of all updates to the actual model. So say you update the description, the actual model will just update to the new description, but the historical model will have a row containing the old description and a row with the new description. Essentially, the historical model.latest() == model.get() if that makes sense.

@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/entrance-exam-support-aa-1205 branch 7 times, most recently from ede4c38 to 5ac308c Compare March 9, 2022 23:10
if draft_node.module.location not in parent.children:
if not hasattr(parent, 'children') or draft_node.module.location not in parent.children:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this change happened because I started seeing some flakey test errors when running the test_mongo.py tests (specifically the test_export_course_image and test_export_course_image_nondefault tests). The error would show up as

if draft_node.module.location not in parent.children:
AttributeError: 'HiddenDescriptorWithMixins' object has no attribute 'children'

But I have no idea how or why it would present itself in this way. It feels like maybe something breaks during the import step (see above in this same file during setUpClass), but I still don't understand how it results in this output.

This change felt like a safe protection regardless as it is just ensuring the attribute exists prior to accessing it, but happy to hear thoughts here.

@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/entrance-exam-support-aa-1205 branch 5 times, most recently from 9218176 to b107762 Compare March 11, 2022 18:11
* Adds entrance exam information to the Course Overview object
    * Enables hiding other tabs since the get_course_tab_list uses
      a Course Overview
    * Enables using the entrance exam helper functions to determine
      if Entrance exams are being used in this course.
* Posts a message when Entrance Exam is passed to parent container for
usage in the Learning MFE
* Overrides the 'title' field of the courseware tab since the Learning MFE
uses that over the 'name' field.
@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/entrance-exam-support-aa-1205 branch from b107762 to d43ece5 Compare March 14, 2022 13:04
@Dillon-Dumesnil Dillon-Dumesnil merged commit a5e51d0 into master Mar 14, 2022
@Dillon-Dumesnil Dillon-Dumesnil deleted the ddumesnil/entrance-exam-support-aa-1205 branch March 14, 2022 13:32
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants