feat: Add platform model and associated fields in Course and Program models #2699

asadali145 · 2023-08-08T12:31:25Z

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

Adds a new model named Platform for course/program partners. Also, adds foreign key fields in the Course and Program models.

How should this be manually tested?

Visit Django admin and verify that Platforms is listed.
Add/Edit any course/program and verify that the platform field is visible.
Verify that platform field is added in the APIs

arslanashraf7

I think we should also think ahead of time while adding this.

There are two things that are coming to my mind right now:

Let's confirm if this is something we want the course team to manage in which case we might need to allow Partner model manipulation and association through Wagtail/CMS. Just like how we manage topics Django model through CMS. This will allow the course team to manage partners through Wagtail without involving the engineering team.
Some time ago we added a partner logo in certificates at Partner logo in certificate template #2407. This is time we should think if we want to move that to this partner model.

What are your thoughts on this. @pdpinch @cachob

arslanashraf7 · 2023-08-11T09:55:51Z

courses/models.py

@@ -209,6 +209,17 @@ def catalog_image_url(self):
 )


+class Partner(models.Model):


Let's inherit this from TimestampedModel

cachob · 2023-08-15T14:27:38Z

Let's confirm if this is something we want the course team to manage in which case we might need to allow Partner model manipulation and association through Wagtail/CMS. Just like how we manage topics Django model through CMS. This will allow the course team to manage partners through Wagtail without involving the engineering team.

I suggest that we keep the control in Django, and not on the CMS. This is a setting that should rarely be touched since we do not get a lot of new external partners.

Some time ago we added a partner logo in certificates at Partner logo in certificate template #2407. This is time we should think if we want to move that to this partner model.

Marketing does not want branding on the course catalog of the xPRO site. So we won't be needing this at least at the moment. Is this a complex thing to add?

arslanashraf7 · 2023-08-18T07:57:17Z

Marketing does not want branding on the course catalog of the xPRO site. So we won't be needing this at least at the moment. Is this a complex thing to add?

@cachob I think we can migrate the existing partners' logs if/when needed. In my opinion that should not be very hard, What I can think of right now is: (When we will need to associate partners with Logo)

Create a model in Wagtail e.g. PartnerLogo Having a foreign key from the Partner Django model and and Image field.
Allow CertificatePage to have a foreign key of PartnerLogo instead of the image field that it currently has
Refactor the code around certificates to use this model
Properly migrate exiting partner logos from certificate page.

arslanashraf7

Just a couple of minor changes suggested.
I haven't tested it yet. I'll test it in the final review. Thanks for your patience on this.

arslanashraf7 · 2023-08-18T08:00:55Z

courses/models.py

+    Model for course partner
+    """
+
+    name = models.CharField(max_length=255)


Let's make it unique(case-insensitive), I don't think we might need to ever support partners with the same name.

arslanashraf7 · 2023-08-18T08:05:39Z

courses/admin.py

@@ -377,6 +375,14 @@ class CourseTopicAdmin(ModelAdmin, admin.ModelAdmin):
    delete_view_class = TopicsWagtailDeleteView


+class PartnerAdmin(admin.ModelAdmin):


I think this should inherit from TimestampedModelAdmin to reuse its display properties, I could not see date fields on the partner detail page in Django Admin.

arslanashraf7 · 2023-08-18T08:06:29Z

courses/factories.py

+class PartnerFactory(DjangoModelFactory):
+    """Factory for Company"""
+
+    name = factory.Faker("partner")


Once we make the partner name unique, This will also be changed to have a prepending dynamic/random name.

arslanashraf7 · 2023-08-18T08:10:35Z

courses/models.py

@@ -220,6 +231,9 @@ class Program(TimestampedModel, PageProperties, ValidateOnSaveMixin):
    live = models.BooleanField(default=False)
    products = GenericRelation(Product, related_query_name="programs")
    is_external = models.BooleanField(default=False)
+    partner = models.ForeignKey(


Ideally, We should have had a common model e.g. Courseware having all the common fields from Course, Program models e.g. live, title, readable_id. And the Course, Program models would inherit from that. Let me know what you think. Maybe we can keep this here for now and do a full refactor of all these in another PR?

Yes, I think we should do this in a separate PR.

arslanashraf7 · 2023-08-18T08:12:34Z

courses/serializers.py

@@ -187,6 +188,10 @@ def get_format(self, instance):  # pylint: disable=unused-argument
        # configurable.
        return "Online"

+    def get_partner(self, instance):
+        """Returns the partner of the course"""
+        return instance.partner.name if instance.partner else None


nit:

Suggested change

return instance.partner.name if instance.partner else None

return getattr(instance.partner, "name", None)

arslanashraf7 · 2023-08-18T08:13:43Z

courses/serializers.py

@@ -187,6 +188,10 @@ def get_format(self, instance):  # pylint: disable=unused-argument
        # configurable.
        return "Online"

+    def get_partner(self, instance):
+        """Returns the partner of the course"""


Suggested change

"""Returns the partner of the course"""

"""Returns the partner name of the course"""

arslanashraf7 · 2023-08-21T22:44:41Z

courses/models.py

+        Validates case insensitive partner name uniqueness.
+        """
+        if (
+            self._state.adding


This should also be enabled for editing mode, Because you can still produce case insensitive partners by in edit mode.

In addition, This doesn't work through the shell. I tried and I could create multiple partners with the same name.

courses/models.py

arslanashraf7

👍 Looks good. Thanks for your patience on this.

arslanashraf7 · 2023-08-29T08:27:28Z

courses/models_test.py

+    with pytest.raises(ValidationError):
+        PartnerFactory.create(name="emeritus")
+
+    PartnerFactory.create(name="Emirates")


nit: This line/creation might not be needed. I think the above assertion is enough.

pdpinch · 2023-08-29T10:12:50Z

Please hold off on merging this. I want to review the field name and make sure it's aligned with field names in MIT Open.

pdpinch · 2023-08-29T18:22:52Z

courses/migrations/0036_add_partner_model.py

+
+    operations = [
+        migrations.CreateModel(
+            name="Partner",


For consistency with MIT Open, can we call this "Platform" instead of "Partner"

Sorry I didn't bring this up earlier.

asadali145 · 2023-08-30T10:28:00Z

@pdpinch Could you please have a look into this PR now?

asadali145 requested a review from arslanashraf7 August 8, 2023 12:31

asadali145 added the Needs Review label Aug 8, 2023

odlbot temporarily deployed to xpro-ci-pr-2699 August 8, 2023 12:32 Inactive

asadali145 removed the Needs Review label Aug 8, 2023

odlbot deployed to xpro-ci-pr-2699 August 8, 2023 12:50 View deployment

asadali145 removed the request for review from arslanashraf7 August 8, 2023 12:50

asadali145 added the Work in Progress label Aug 8, 2023

odlbot had a problem deploying to xpro-ci-pr-2699 August 9, 2023 06:59 Failure

odlbot had a problem deploying to xpro-ci-pr-2699 August 9, 2023 07:32 Failure

asadali145 requested a review from arslanashraf7 August 9, 2023 08:51

asadali145 added Needs Review and removed Work in Progress labels Aug 9, 2023

arslanashraf7 self-assigned this Aug 10, 2023

arslanashraf7 reviewed Aug 11, 2023

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 11, 2023

asadali145 requested a review from arslanashraf7 August 16, 2023 11:59

asadali145 added Needs Review and removed Waiting on Author labels Aug 16, 2023

arslanashraf7 requested changes Aug 18, 2023

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 18, 2023

asadali145 requested a review from arslanashraf7 August 21, 2023 12:46

asadali145 added Needs Review and removed Waiting on Author labels Aug 21, 2023

arslanashraf7 requested changes Aug 22, 2023

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 23, 2023

asadali145 added 9 commits August 28, 2023 16:42

updated migration name

7ba0e82

use timestamped model

b9f747a

fix test

6984f46

fix test

b191f84

review changes

b76fbc6

review changes

f709b87

review changes

b62a957

deleted migration

c3d3fd1

recreated migration

d9ce219

asadali145 force-pushed the asad/add-partner-model branch from 558aac7 to d9ce219 Compare August 28, 2023 11:48

lint

2390e0f

asadali145 requested a review from arslanashraf7 August 28, 2023 12:14

asadali145 added Needs Review and removed Waiting on Author labels Aug 28, 2023

arslanashraf7 approved these changes Aug 29, 2023

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 29, 2023

pdpinch requested changes Aug 29, 2023

View reviewed changes

refactor: rename partner model to platform

bb16eab

asadali145 changed the title ~~feat: Add partner model and associated fields in Course and Program models~~ feat: Add platform model and associated fields in Course and Program models Aug 30, 2023

asadali145 requested a review from arslanashraf7 August 30, 2023 08:10

asadali145 added Needs Review and removed Waiting on Author labels Aug 30, 2023

feat: add platform in program serializer

7dbfb89

asadali145 requested a review from pdpinch August 30, 2023 10:27

arslanashraf7 approved these changes Aug 31, 2023

View reviewed changes

asadali145 merged commit 51ac860 into master Aug 31, 2023
3 checks passed

odlbot mentioned this pull request Aug 31, 2023

Release 0.127.1 #2717

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add platform model and associated fields in Course and Program models #2699

feat: Add platform model and associated fields in Course and Program models #2699

asadali145 commented Aug 8, 2023 •

edited

Loading

arslanashraf7 left a comment

arslanashraf7 Aug 11, 2023

cachob commented Aug 15, 2023

arslanashraf7 commented Aug 18, 2023

arslanashraf7 left a comment

arslanashraf7 Aug 18, 2023

arslanashraf7 Aug 18, 2023

arslanashraf7 Aug 18, 2023

arslanashraf7 Aug 18, 2023

asadali145 Aug 21, 2023

arslanashraf7 Aug 18, 2023

arslanashraf7 Aug 18, 2023

arslanashraf7 Aug 21, 2023

arslanashraf7 Aug 21, 2023

arslanashraf7 left a comment

arslanashraf7 Aug 29, 2023

pdpinch commented Aug 29, 2023

pdpinch Aug 29, 2023

asadali145 commented Aug 30, 2023

		@@ -209,6 +209,17 @@ def catalog_image_url(self):
		)


		class Partner(models.Model):

		@@ -377,6 +375,14 @@ class CourseTopicAdmin(ModelAdmin, admin.ModelAdmin):
		delete_view_class = TopicsWagtailDeleteView


		class PartnerAdmin(admin.ModelAdmin):

	return instance.partner.name if instance.partner else None
	return getattr(instance.partner, "name", None)

	"""Returns the partner of the course"""
	"""Returns the partner name of the course"""

feat: Add platform model and associated fields in Course and Program models #2699

feat: Add platform model and associated fields in Course and Program models #2699

Conversation

asadali145 commented Aug 8, 2023 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cachob commented Aug 15, 2023

arslanashraf7 commented Aug 18, 2023

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdpinch commented Aug 29, 2023

Choose a reason for hiding this comment

asadali145 commented Aug 30, 2023

asadali145 commented Aug 8, 2023 •

edited

Loading