Skip to content

Bszabo/tnl 10136 student course enrollment - #31229

Closed
bszabo wants to merge 15 commits into
masterfrom
bszabo/TNL-10136-student-course-enrollment
Closed

Bszabo/tnl 10136 student course enrollment#31229
bszabo wants to merge 15 commits into
masterfrom
bszabo/TNL-10136-student-course-enrollment

Conversation

@bszabo

@bszabo bszabo commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

This is an incremental step towards teasing apart a course enrollment model out of the existing student model in edx-platform. These changes are intended to be non-breaking. Please fail the PR if that objective goes unmet.

Model code had previously all resided in a single student/models.py module. Now it resides in two modules, one for student modeling and one for course enrollment modeling.

Database tables remain as is.

Importing of student and course enrollment models remain as is.

@bszabo
bszabo force-pushed the bszabo/TNL-10136-student-course-enrollment branch from e718de4 to eec0c6a Compare October 28, 2022 19:07
@bszabo
bszabo requested review from connorhaugh and ormsbee October 28, 2022 19:08
from openedx.features.enterprise_support.tests.factories import EnterpriseCustomerUserFactory
from common.djangoapps.student.models import LoginFailures
from common.djangoapps.util.password_policy_validators import DEFAULT_MAX_PASSWORD_LENGTH
from openedx_events.tests.utils import OpenEdxEventsTestMixin # lint-amnesty, pylint: disable=wrong-import-order

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.

Why do we want this imported at this line in particular?

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.

I used a PyCharm "Optimize Imports" tool action, which, among other things, arranges imports alphabetically. It would appear that the underscore in "openedx_events" places this import in lower alphabetic order relative to the dot in "openedex.features"

@connorhaugh

Copy link
Copy Markdown
Contributor

Great use of atomic commits here it makes reading the story of this revision much easier.

Comment thread common/djangoapps/student/tests/test_enrollment.py
@bszabo
bszabo requested a review from iloveagent57 November 14, 2022 12:39
@bszabo

bszabo commented Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

In side channel slack conversation, Alexander confirmed that none of these changes broke the edx-enterprise build, and that the extraction of the course enrollment from the student model looks largely correct (suitable first approximation).

@bszabo
bszabo force-pushed the bszabo/TNL-10136-student-course-enrollment branch from eec0c6a to b497a7d Compare November 16, 2022 20:22
Jenkins and others added 11 commits November 16, 2022 15:48
See details in the related PR

Co-authored-by: Omar Al-Ithawi <i@omardo.com>
Co-authored-by: Shadi Naif <shadinaif@gmail.com>
fix imports and mocking to match new home
All tests now pass.
Lots of temporary diagnostic logging statements in place; still need removal
feat: TNL-10136 Remove diagnostic print statements


Statements were added as part of fixing broken unit tests
@connorhaugh

Copy link
Copy Markdown
Contributor

@bszabo If you could hi light any changes required by the merge, I would be happy to give this a second thumb upon review of those changes.

@e0d

e0d commented Nov 20, 2023

Copy link
Copy Markdown
Contributor

Hi @bszabo should this PR be closed?

@feanil

feanil commented Apr 24, 2024

Copy link
Copy Markdown
Contributor

Closing since there hasn't been a response in a while.

@feanil feanil closed this Apr 24, 2024
@feanil
feanil deleted the bszabo/TNL-10136-student-course-enrollment branch April 24, 2024 16:13
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.

6 participants