PADV-1418 feat: adds a new custom content jsonfield to ccx model #133

JuanDavidBuitrago · 2024-10-04T13:49:58Z

Description

This PR adds a new custom content JSONField to the CCX CustomCourseForEdX model, in order to use this field to storage any additional information that we need to associate with.

Testing instructions

Using your local environment with this branch, go to the LMS container and run the commands.

python manage.py lms makemigrations
python manage.py lms migrate

Go to http://{base_url}/api/ccx/v0/ccx/?master_course_id={master_course_id} to see the list of CCX courses associate to your master course. Look for the other_course_settings field for the CCX in the list.

{
   "next":null,
   "previous":null,
   "count":7,
   "num_pages":1,
   "current_page":1,
   "start":0,
   "results":[
   {
      "ccx_course_id":"ccx-v1:demo+cs100+2024-t2+ccx@1",
      "master_course_id":"course-v1:demo+cs100+2024-t2",
      "display_name":"skillable edunext test ccx",
      "coach_email":"[email protected]",
      "start":"2024-09-24 13:17:07+00:00",
      "due":null,
      "max_students_allowed":200,
      "course_modules":[],
      "other_course_settings":{}
   }, ...]}

Go to http://{base_url}/api/ccx/v0/ccx/{ccx_course_id} to see the details for a specific CCX course. Again, look for the other_course_settings field
Using Postman, you can test PUT and PATCH request to add/modify the other_course_settings field.

Note: Here is a Postman collection to import and test easily
Custom content XL-local.postman_collection.json

Jira issue

https://agile-jira.pearson.com/browse/PADV-1418

Squirrel18 · 2024-10-04T22:02:50Z

lms/djangoapps/ccx/models.py

@@ -32,6 +32,8 @@ class CustomCourseForEdX(models.Model):
    # if not empty, this field contains a json serialized list of
    # the master course modules
    structure_json = models.TextField(verbose_name='Structure JSON', blank=True, null=True)
+    # Custom Json field to add any additional information to the CCX model
+    custom_content = models.JSONField(default=dict, blank=True, null=True, verbose_name="Custom Json Content")


I think we should name it the same as "other course settings", as they both have the same purpose and having the same name could easily guide the people who will work with this. What do you think? @JuanDavidBuitrago

@Squirrel18 I think it's a good idea, and it makes a lot of sense. Variable name changed.

Squirrel18 · 2024-10-04T22:05:20Z

lms/djangoapps/ccx/api/v0/serializers.py

@@ -52,3 +54,10 @@ def get_course_modules(obj):
        Getter for the Course Modules. The list is stored in a compressed field.
        """
        return obj.structure or []
+
+    @staticmethod
+    def get_custom_content(obj):


I think this is not necessary, you could use a DictField, why do you need this? @JuanDavidBuitrago

@Squirrel18 I have updated the serializer to remove the static method get_custom_content, as other_course_settings is a JSONField in the model that already handles default values and ensures it will always return a dictionary. Thanks for the suggestion!

nandodev-net · 2024-10-07T13:55:25Z

On Friday we held a meeting to try to test this PR, the migrations are not recognized by my environment in devstack, in fact it affected the CCX configuration in a negative way. I suggest reviewing and updating the How to Test.

JuanDavidBuitrago · 2024-10-07T14:02:17Z

On Friday we held a meeting to try to test this PR, the migrations are not recognized by my environment in devstack, in fact it affected the CCX configuration in a negative way. I suggest reviewing and updating the How to Test.

@nandodev-net The problem is in your local environment, so I shouldn't modify my instructions because there is nothing else I can do from my side.

nandodev-net · 2024-10-07T15:50:10Z

It's not a branch problem. Once the configuration for the CCX was redone, the migrations ran without problem.

Thanks for the postman collection, I think it's an excellent idea to facilitate testing in PRs! :)

Squirrel18 · 2024-10-07T19:58:32Z

lms/djangoapps/ccx/models.py

@@ -32,6 +32,8 @@ class CustomCourseForEdX(models.Model):
    # if not empty, this field contains a json serialized list of
    # the master course modules
    structure_json = models.TextField(verbose_name='Structure JSON', blank=True, null=True)
+    # Custom Json field to add any additional information to the CCX model


Period at the end of the line. @JuanDavidBuitrago

Squirrel18 · 2024-10-07T19:59:01Z

lms/djangoapps/ccx/models.py

@@ -32,6 +32,8 @@ class CustomCourseForEdX(models.Model):
    # if not empty, this field contains a json serialized list of
    # the master course modules
    structure_json = models.TextField(verbose_name='Structure JSON', blank=True, null=True)
+    # Custom Json field to add any additional information to the CCX model
+    other_course_settings = models.JSONField(default=dict, blank=True, null=True, verbose_name="Custom Json Content")


Suggested change

other_course_settings = models.JSONField(default=dict, blank=True, null=True, verbose_name="Custom Json Content")

other_course_settings = models.JSONField(default=dict(), blank=True, null=True, verbose_name='Custom Json Content')

lms/djangoapps/ccx/api/v0/serializers.py

Squirrel18 · 2024-10-07T20:30:08Z

lms/djangoapps/ccx/api/v0/views.py

@@ -733,6 +761,10 @@ def patch(self, request, ccx_course_id=None):
                if ccx_course_object.coach.id != coach.id:
                    old_coach = ccx_course_object.coach
                    ccx_course_object.coach = coach
+            if 'other_course_settings' in valid_input:
+                existing_content = ccx_course_object.other_course_settings
+                existing_content.update(valid_input.get('other_course_settings'))


Please, remove this update. @JuanDavidBuitrago

JuanDavidBuitrago · 2024-10-07T21:41:29Z

lms/djangoapps/ccx/api/v0/views.py

@@ -152,6 +152,13 @@ def get_valid_input(request_data, ignore_missing=False):
    elif 'max_students_allowed' in request_data:
        field_errors['max_students_allowed'] = {'error_code': 'null_field_max_students_allowed'}

+    other_course_settings = request_data.get('other_course_settings')
+    if other_course_settings is not None:


@Squirrel18 at the end we need to validate if exist in a different level, because when the request doesn't have the value, it returns the field error even though we don't need to add/change it

In this case can't we use: if other_course_settings: and process it if it's present? @JuanDavidBuitrago

Squirrel18 · 2024-10-07T22:50:55Z

lms/djangoapps/ccx/api/v0/views.py

@@ -152,6 +152,13 @@ def get_valid_input(request_data, ignore_missing=False):
    elif 'max_students_allowed' in request_data:
        field_errors['max_students_allowed'] = {'error_code': 'null_field_max_students_allowed'}

+    other_course_settings = request_data.get('other_course_settings')
+    if other_course_settings is not None:


In this case can't we use: if other_course_settings: and process it if it's present? @JuanDavidBuitrago

Squirrel18 · 2024-10-07T22:51:30Z

lms/djangoapps/ccx/api/v0/serializers.py

@@ -19,6 +19,7 @@ class CCXCourseSerializer(serializers.ModelSerializer):
    due = serializers.CharField(allow_blank=True)
    max_students_allowed = serializers.IntegerField(source='max_student_enrollments_allowed')
    course_modules = serializers.SerializerMethodField()
+    other_course_settings = serializers.JSONField()


As it's now, this field is required: https://www.django-rest-framework.org/api-guide/fields/#required @JuanDavidBuitrago

JuanDavidBuitrago requested review from kuipumu, nandodev-net and Squirrel18 October 4, 2024 13:49

JuanDavidBuitrago self-assigned this Oct 4, 2024

Squirrel18 requested changes Oct 4, 2024

View reviewed changes

JuanDavidBuitrago force-pushed the vue/PADV-1418 branch from ffff35f to 23ac007 Compare October 7, 2024 11:18

JuanDavidBuitrago requested a review from Squirrel18 October 7, 2024 11:26

nandodev-net approved these changes Oct 7, 2024

View reviewed changes

Squirrel18 requested changes Oct 7, 2024

View reviewed changes

JuanDavidBuitrago commented Oct 7, 2024

View reviewed changes

JuanDavidBuitrago force-pushed the vue/PADV-1418 branch from baa9ac9 to 2fc55c1 Compare October 7, 2024 21:43

JuanDavidBuitrago requested a review from Squirrel18 October 7, 2024 21:44

Squirrel18 requested changes Oct 7, 2024

View reviewed changes

JuanDavidBuitrago requested a review from Squirrel18 October 8, 2024 08:06

JuanDavidBuitrago force-pushed the vue/PADV-1418 branch from d0b9885 to 5381560 Compare October 8, 2024 11:39

Squirrel18 approved these changes Oct 8, 2024

View reviewed changes

kuipumu approved these changes Oct 8, 2024

View reviewed changes

JuanDavidBuitrago force-pushed the vue/PADV-1418 branch from 5381560 to 5fc8db7 Compare October 8, 2024 15:50

feat: adds a new custom content jsonfield to ccx model

f995679

JuanDavidBuitrago force-pushed the vue/PADV-1418 branch from 5fc8db7 to f995679 Compare October 8, 2024 16:03

JuanDavidBuitrago merged commit 47fbb31 into pearson-release/olive.stage Oct 8, 2024
13 of 35 checks passed

JuanDavidBuitrago mentioned this pull request Oct 9, 2024

PADV-1418 feat: adds a new custom content jsonfield to ccx model #134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PADV-1418 feat: adds a new custom content jsonfield to ccx model #133

PADV-1418 feat: adds a new custom content jsonfield to ccx model #133

JuanDavidBuitrago commented Oct 4, 2024 •

edited

Loading

Squirrel18 Oct 4, 2024

JuanDavidBuitrago Oct 7, 2024

Squirrel18 Oct 4, 2024

JuanDavidBuitrago Oct 7, 2024

nandodev-net commented Oct 7, 2024

JuanDavidBuitrago commented Oct 7, 2024

nandodev-net commented Oct 7, 2024 •

edited

Loading

Squirrel18 Oct 7, 2024

Squirrel18 Oct 7, 2024

Squirrel18 Oct 7, 2024

JuanDavidBuitrago Oct 7, 2024

Squirrel18 Oct 7, 2024

Squirrel18 Oct 7, 2024

Squirrel18 Oct 7, 2024

	other_course_settings = models.JSONField(default=dict, blank=True, null=True, verbose_name="Custom Json Content")
	other_course_settings = models.JSONField(default=dict(), blank=True, null=True, verbose_name='Custom Json Content')

PADV-1418 feat: adds a new custom content jsonfield to ccx model #133

PADV-1418 feat: adds a new custom content jsonfield to ccx model #133

Conversation

JuanDavidBuitrago commented Oct 4, 2024 • edited Loading

Description

Testing instructions

Jira issue

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

nandodev-net commented Oct 7, 2024

JuanDavidBuitrago commented Oct 7, 2024

nandodev-net commented Oct 7, 2024 • edited Loading

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

JuanDavidBuitrago commented Oct 4, 2024 •

edited

Loading

nandodev-net commented Oct 7, 2024 •

edited

Loading