feat: [FC-0031] Add field profile_image to CommentSerializer #33295

oksana-slu · 2023-09-20T11:10:41Z

Description

This PR extends response for POST request for creating a comment with the data about profile image. This way, it's possible to fetch an avatar image for a post or response without an additional request to a separate account settings endpoint.

Supporting information

This contribution is a part of the project FC-0031 https://openedx.atlassian.net/wiki/spaces/COMM/pages/3844505604/FC-0031+-+Mobile+API+Migration

Testing instructions

POST /api/discussion/v1/comments/ with body {"thread_id": "put here thread id", "raw_body": "test message"}

Check that profile_image field present in response.

openedx-webhooks · 2023-09-20T11:10:47Z

Thanks for the pull request, @oksana-slu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

saeedbashir · 2023-09-25T08:39:25Z

I'm not sure either it's relevant or not but discussion APIs' are returning the user-profile data on demand when we request it by passing the additional query parameter profile_image in requested_fields.

The following API endpoints respect the query parameter specified above:

/api/discussion/v1/comments/
/api/discussion/v1/course_topics/
/api/discussion/v1/threads/

marcotuts · 2023-09-25T19:12:24Z

I see reference to "AccountLegacyProfileSerializer" and it made me wonder if we need to look at both edx-platform as this PR does and the new profile MFE, or do both rely on the same endpoints / data?

In other words if the legacy edx-platform profile views are removed would we need to do anything else to have the apps still be able to request the profile image?

(edit: This is for discussions, my comment was related to profile pages not discussion directly. feel free to ignore for the purpose of this PR)

moeez96 · 2023-10-04T09:57:57Z

lms/djangoapps/discussion/rest_api/serializers.py

@@ -502,6 +503,7 @@ class CommentSerializer(_ContentSerializer):
    child_count = serializers.IntegerField(read_only=True)
    children = serializers.SerializerMethodField(required=False)
    abuse_flagged_any_user = serializers.SerializerMethodField(required=False)
+    profile_image = serializers.SerializerMethodField(read_only=True)


As mentioned by @saeedbashir here, Can we use the requested_fields parameters to obtain similar results in the response? The use is mentioned in code here: https://github.com/openedx/edx-platform/blob/dc5d225e920e68b7dd4526a812e60e0b808d344c/lms/djangoapps/discussion/rest_api/views.py#L798

@oksana-slu I'm curious about your response to this, if there are specific discussion endpoints that don't support requested_fields can we add that option to them instead? This seems more robust and better matches the other code in this API.

@moeez96 @feanil
Please confirm that we correctly understood the proposed solution.

The goal of this PR is to return profile_image field in response to POST request to /api/discussion/v1/comments/ (When a new comment is created).
Do you propose to extend POST method of the view, so we can provide list of fields, requested_fields in the body of the POST request, similar to GET request, and then determine what additional fields should be included in the response based on provided list?

@GlugovGrGlib I think I mis-understood, I think it's fine to include this URL though I'm concerned about the coupling/dependency to a serializer in the user_api djangoapp. I'll make a separate comment and suggestion for that.

feanil

Looking for feedback from the author on the suggestion made regarding requested_fields

feanil · 2023-10-12T20:54:54Z

lms/djangoapps/discussion/rest_api/serializers.py

@@ -502,6 +503,7 @@ class CommentSerializer(_ContentSerializer):
    child_count = serializers.IntegerField(read_only=True)
    children = serializers.SerializerMethodField(required=False)
    abuse_flagged_any_user = serializers.SerializerMethodField(required=False)
+    profile_image = serializers.SerializerMethodField(read_only=True)


@oksana-slu I'm curious about your response to this, if there are specific discussion endpoints that don't support requested_fields can we add that option to them instead? This seems more robust and better matches the other code in this API.

feanil

Requesting some changes so that we're not reaching into the serializers of the user_api djangoapp.

feanil · 2023-11-07T16:34:08Z

lms/djangoapps/discussion/rest_api/serializers.py

@@ -502,6 +503,7 @@ class CommentSerializer(_ContentSerializer):
    child_count = serializers.IntegerField(read_only=True)
    children = serializers.SerializerMethodField(required=False)
    abuse_flagged_any_user = serializers.SerializerMethodField(required=False)
+    profile_image = serializers.SerializerMethodField(read_only=True)


@GlugovGrGlib I think I mis-understood, I think it's fine to include this URL though I'm concerned about the coupling/dependency to a serializer in the user_api djangoapp. I'll make a separate comment and suggestion for that.

feanil · 2023-11-07T16:39:55Z

lms/djangoapps/discussion/rest_api/serializers.py

@@ -584,6 +586,10 @@ def get_abuse_flagged_any_user(self, obj):
        if _validate_privileged_access(self.context):
            return len(obj.get("abuse_flaggers", [])) > 0

+    def get_profile_image(self, obj):
+        request = self.context["request"]
+        return AccountLegacyProfileSerializer.get_profile_image(request.user.profile, request.user, request)


I'm concerned about the coupling of serializers in the user_api djangoapp with apis in the discussions app. I think it's pretty reasonable that we want to be able to send back profile image data from multiple places so I suggest that we extract the get_profile_images function to the api.py file in the user_api django app so it's clear that others are expected to use this and that it forms an interface that should not break.

See https://github.com/openedx/edx-platform/blob/master/docs/decisions/0002-inter-app-apis.rst for the reasoning behind this suggestion.

Done. I moved get_profile_images method to api. Now we don't use method from AccountLegacyProfileSerializer in CommentSerializer.

feanil

Let's make the docstring more useful. Also, this is duplicating the code from where it was previously, does it make more sense for this to just be a pass-through call to the other function. The goal of having it in api.py is to make it clear that it can and will be used externally but we don't need to duplicate the code I don't think.

feanil · 2023-11-08T20:54:08Z

openedx/core/djangoapps/user_api/accounts/api.py

+    """
+    Returns metadata about a user's profile image.
+    """


Suggested change

"""

Returns metadata about a user's profile image.

"""

"""

Returns metadata about a user's profile image.

The output is a dict that looks like:

{

"has_image": False,

"image_url_full": "http://testserver/static/default_500.png",

"image_url_large": "http://testserver/static/default_120.png",

"image_url_medium": "http://testserver/static/default_50.png",

"image_url_small": "http://testserver/static/default_30.png",

}

"""

Added additional description to the docstring

feanil · 2023-11-13T20:19:41Z

openedx/core/djangoapps/user_api/accounts/serializers.py

@@ -38,7 +38,7 @@
    PRIVATE_VISIBILITY,
    VISIBILITY_PREFIX
 )
-from .image_helpers import get_profile_image_urls_for_user


The serializers module gets imported a lot from the api module already so it's leading to a circular import. I think you should extract the function to utils.py and then call it from both the API and from the serializer to resolve the issue.

Sure, thanks, updated code

There is still a problem with circular imports when placing the code in utils.py, do you think the proposed implementation using AccountLegacyProfileSerializer in api.py is appropriate?

you mean calling the class method from api.py? I think that should be okay.

Yes, exactly
I updated api.py and rebased branch to the latest master, please do the final review

feanil

Looks good, I'll merge in the morning(ET) so I can keep an eye on it while it goes to edx.org and make sure we didn't miss anything.

openedx-webhooks · 2023-11-16T14:57:07Z

@oksana-slu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-11-16T16:06:45Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-11-16T16:35:17Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-11-16T16:35:18Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-11-16T16:43:19Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-11-16T17:02:47Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-11-16T17:02:48Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 20, 2023

oksana-slu marked this pull request as ready for review September 20, 2023 16:27

GlugovGrGlib force-pushed the rg/feat/FC0031/add_profile_image_to_api_discussion branch from ed4144b to dc5d225 Compare September 22, 2023 09:07

marcotuts mentioned this pull request Sep 27, 2023

Mobile API Updates (FC-0031) #33304

Closed

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 2, 2023

moeez96 reviewed Oct 4, 2023

View reviewed changes

feanil reviewed Oct 12, 2023

View reviewed changes

feanil requested changes Nov 7, 2023

View reviewed changes

feanil reviewed Nov 9, 2023

View reviewed changes

GlugovGrGlib force-pushed the rg/feat/FC0031/add_profile_image_to_api_discussion branch from ca2cf6e to a3d0a3e Compare November 13, 2023 10:27

feanil reviewed Nov 13, 2023

View reviewed changes

GlugovGrGlib force-pushed the rg/feat/FC0031/add_profile_image_to_api_discussion branch 4 times, most recently from 1bcfa84 to dbddec7 Compare November 13, 2023 22:43

KyryloKireiev and others added 3 commits November 14, 2023 21:45

feat: [AXIM-20] Add profile_image to API CommentViewSet

ae3830b

refactor: [FC-0031] Move get_profile_image method to api

da9266e

fix: remove duplicate implementation and correct the docstring

e27f674

GlugovGrGlib force-pushed the rg/feat/FC0031/add_profile_image_to_api_discussion branch from dbddec7 to e27f674 Compare November 14, 2023 19:45

feanil approved these changes Nov 14, 2023

View reviewed changes

feanil merged commit 942a241 into openedx:master Nov 16, 2023

GlugovGrGlib deleted the rg/feat/FC0031/add_profile_image_to_api_discussion branch December 14, 2023 16:36

OmarIthawi mentioned this pull request May 31, 2024

feat: cherry pick mobile apis nelc/edx-platform#29

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [FC-0031] Add field profile_image to CommentSerializer #33295

feat: [FC-0031] Add field profile_image to CommentSerializer #33295

oksana-slu commented Sep 20, 2023 •

edited

Loading

openedx-webhooks commented Sep 20, 2023 •

edited

Loading

saeedbashir commented Sep 25, 2023

marcotuts commented Sep 25, 2023 •

edited

Loading

moeez96 Oct 4, 2023

feanil Oct 12, 2023

GlugovGrGlib Nov 3, 2023

feanil Nov 7, 2023

feanil left a comment

feanil Oct 12, 2023

feanil left a comment

feanil Nov 7, 2023

feanil Nov 7, 2023

KyryloKireiev Nov 8, 2023

feanil left a comment

feanil Nov 8, 2023

GlugovGrGlib Nov 13, 2023

feanil Nov 13, 2023

GlugovGrGlib Nov 13, 2023

GlugovGrGlib Nov 13, 2023

feanil Nov 14, 2023

GlugovGrGlib Nov 14, 2023

feanil left a comment

openedx-webhooks commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

-    """
-    Returns metadata about a user's profile image.
-    """
+    """
+    Returns metadata about a user's profile image.
+    The output is a dict that looks like:
+    {
+        "has_image": False,
+        "image_url_full": "http://testserver/static/default_500.png",
+        "image_url_large": "http://testserver/static/default_120.png",
+        "image_url_medium": "http://testserver/static/default_50.png",
+        "image_url_small": "http://testserver/static/default_30.png",
+    }
+    """

feat: [FC-0031] Add field profile_image to CommentSerializer #33295

feat: [FC-0031] Add field profile_image to CommentSerializer #33295

Conversation

oksana-slu commented Sep 20, 2023 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Sep 20, 2023 • edited Loading

saeedbashir commented Sep 25, 2023

marcotuts commented Sep 25, 2023 • 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

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feanil 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

feanil 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

feanil left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

edx-pipeline-bot commented Nov 16, 2023

oksana-slu commented Sep 20, 2023 •

edited

Loading

openedx-webhooks commented Sep 20, 2023 •

edited

Loading

marcotuts commented Sep 25, 2023 •

edited

Loading