feat: added filter for modifying the isStarted for externally hosted …

[jajjibhai008]

Description:
This PR adds a filter for modifying the isStarted for externally hosted courses. This hook will identify whether the learner has started the course or not.

Reference:
Actually, this field will be used to identify whether the learner has started the course or not. And we will conditionally render some things on the learner dashboard (B2C side) as described in ENT-7188.

JIRA- ENT-7373 and ENT-at 7374

Dependencies: dependencies on other outstanding PRs, issues, etc.

Merge deadline: List merge deadline (if any)

Installation instructions: List any non-trivial installation
instructions.

Testing instructions:

1. Open page A
2. Do thing B
3. Expect C to happen
4. If D happened instead - check failed.

Reviewers:

• tag reviewer
• tag reviewer

Merge checklist:

• All reviewers approved
• CI build is green
• Version bumped
• Changelog record added
• Documentation updated (not only docstrings)
• Commits are squashed

Post merge:

• Create a tag
• Check new version is pushed to PyPI after tag-triggered build is
  finished.
• Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

[jajjibhai008]

@felipemontoya could you please review my PR? I have to ship these changes as soon as possible.

[felipemontoya]

Hi @jajjibhai008 , thanks for tagging me.

Do you have the link to the corresponding edx-platform pr? I am not understanding the goal of this filter by looking at the definition alone.

[muhammad-ammar]

Hi @felipemontoya, Here is the edx-platform PR: openedx/edx-platform#32788

[felipemontoya]

Thanks a lot. Now I understand much better.

I think the definition of the filter is too specific to make it a reusable filter.
Also it makes the naming difficult.

You have:
"org.openedx.learning.course.is.started.creation.started.v1"

But in order to truly pinpoint the filtered situation it would have to be:
"org.openedx.learning.home.enrollment.api.render.is.started.calculated.v1"

I think you could refactor this a bit to make it global to the whole API call.

Broadly speaking you could let the serializer run its whole logic and then add a filter that lets you work on the final result.

class EnrollmentSerializer(serializers.Serializer):

    def to_representation(self, instance):
        """Let the filter work after the whole instance is serialized"""
        ret = super().to_representation(instance)
        ret = CourseEnrollmentAPIRendered().run_filter(representation=ret)
        return ret

So you could have a filter whose domain is:
"org.openedx.learning.home.enrollment.api.rendered.v1"

Such a filter could be re used in the future when you want to alter any of the fields of this API

[felipemontoya]

Now that I think of it, maybe you would rather filter the instance object before is passed to to_representation. In that case you are working with actual models instead of text/json in the filter. But the overall idea stays the same.

[felipemontoya]

as per the comment above

[jajjibhai008]

Thanks a lot. Now I understand much better.

I think the definition of the filter is too specific to make it a reusable filter. Also it makes the naming difficult.

You have: "org.openedx.learning.course.is.started.creation.started.v1"

But in order to truly pinpoint the filtered situation it would have to be: "org.openedx.learning.home.enrollment.api.render.is.started.calculated.v1"

I think you could refactor this a bit to make it global to the whole API call.

Broadly speaking you could let the serializer run its whole logic and then add a filter that lets you work on the final result.

class EnrollmentSerializer(serializers.Serializer):

    def to_representation(self, instance):
        """Let the filter work after the whole instance is serialized"""
        ret = super().to_representation(instance)
        ret = CourseEnrollmentAPIRendered().run_filter(representation=ret)
        return ret

So you could have a filter whose domain is: "org.openedx.learning.home.enrollment.api.rendered.v1"

Such a filter could be re used in the future when you want to alter any of the fields of this API

HI @felipemontoya Thanks for your detailed review.

1. I think your suggested filter name is more concise and descriptive, So I have renamed the filter name in all PRs.
2. I agree with your approach to writing the filtration logic in the to_representation(self, instance) method. This will make code more efficient and readable. I have update the platform PR according to your suggestions.
   Please let me know if you have any questions or confusion.

[felipemontoya]

hey @jajjibhai008, I'm happy that the review helped.

I'd still like to understand something. You changed the position of the filter to a position where you can have an effect on all the fields for this API. But you are still defining the filter in a way that would only let a developer using the filter change the is_started value. I would still suggest that you change the definition so that by opening the capability to alter the API you are actually allowing the complete set of fields to be worked on.

Then you could call the filter "org.openedx.learning.home.enrollment.api.rendered.v1" and allow other features in the future to build on top of this.

[jajjibhai008]

hey @jajjibhai008, I'm happy that the review helped.

I'd still like to understand something. You changed the position of the filter to a position where you can have an effect on all the fields for this API. But you are still defining the filter in a way that would only let a developer using the filter change the is_started value. I would still suggest that you change the definition so that by opening the capability to alter the API you are actually allowing the complete set of fields to be worked on.

Then you could call the filter "org.openedx.learning.home.enrollment.api.rendered.v1" and allow other features in the future to build on top of this.

@felipemontoya I have updated the filter definition to CourseEnrollmentRenderStarted. And also I have updated the filter type to "org.openedx.learning.home.enrollment.api.rendered.v1" according to your suggestions.
Please let me know if you have any follow-up questions or suggestions.

[felipemontoya]

Thanks @jajjibhai008. I would say codewise this looks good now.

I don't have access to any of the Jira tickets you linked so I have no Idea about how this fullfills or not the requirement, but I'll leave my review approval

[jajjibhai008]

Thanks, @felipemontoya for your kind and experienced reviews.
As I don't have access to merge it, please merge it for me, so that I can use these changes.

[felipemontoya]

Could someone else from your team that has access to the jira tickets give a review as well? I have no context from the requirements other that what is in this page.

[muhammad-ammar]

Awesome Work!