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: add vertical models in tagging app #4544

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

AfaqShuaib09
Copy link
Contributor

@AfaqShuaib09 AfaqShuaib09 commented Jan 15, 2025

PROD-4280

This PR adds the model related to vertical tagging.

  • Create vertical, subvertical, and coursevertical models in tagging.
  • Register the models on admin.
  • Ensure proper validation checks are present on models save (only superusers and allowed user group)
  • Use history to track changes.

@AfaqShuaib09 AfaqShuaib09 force-pushed the afaq/prod-4280 branch 2 times, most recently from c044fe0 to f4ae8ee Compare January 16, 2025 07:46
@AfaqShuaib09 AfaqShuaib09 changed the base branch from afaq/prod-4279 to master January 16, 2025 10:10
@AfaqShuaib09 AfaqShuaib09 force-pushed the afaq/prod-4280 branch 2 times, most recently from 2293acd to e1a1f9b Compare January 16, 2025 11:00
@AfaqShuaib09 AfaqShuaib09 marked this pull request as ready for review January 17, 2025 11:07
course_discovery/settings/base.py Show resolved Hide resolved
course_discovery/apps/tagging/models.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/models.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/models.py Show resolved Hide resolved
Copy link
Contributor

@zawan-ila zawan-ila left a comment

Choose a reason for hiding this comment

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

Some tests appear to be testing django code more than our code e.g cascading behavior, uniqueness constraints. I do not think such tests add much value but it is OK if you feel otherwise.

course_discovery/apps/tagging/tests/test_models.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/tests/test_models.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/tests/test_admin.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/tests/test_admin.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/tests/test_models.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/tests/test_admin.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/models.py Outdated Show resolved Hide resolved
course_discovery/apps/tagging/models.py Show resolved Hide resolved
Comment on lines 115 to 112
self.coursevertical_admin.save_model(request, self.coursevertical, None, False)

def test_save_model_as_allowed_group_user(self):
"""Verify that user in allowed group can save course vertical."""
self.regular_user.groups.add(self.allowed_group)
request = MockRequest(self.regular_user)
self.coursevertical_admin.save_model(request, self.coursevertical, None, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

same assertion comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, it's only for checking permissions, as it will raise a permission issue in case of a non-allowed user group. I'm now adding some assertions by changing a field

Comment on lines +56 to +60
Vertical, on_delete=models.CASCADE, null=True, blank=True, related_name="%(class)s_verticals"
)
sub_vertical = models.ForeignKey(
SubVertical, on_delete=models.CASCADE, null=True, blank=True, related_name="%(class)s_sub_verticals"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do this %(class)? The migrations do not translate this to anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +96 to +97
if not self.vertical:
self.vertical = self.sub_vertical.vertical # Auto-assign vertical if it's not set
Copy link
Contributor

Choose a reason for hiding this comment

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

missing coverage

@AfaqShuaib09 AfaqShuaib09 merged commit 8ac5a04 into master Jan 21, 2025
12 checks passed
@AfaqShuaib09 AfaqShuaib09 deleted the afaq/prod-4280 branch January 21, 2025 11:25
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.

3 participants