fix: ORA response with attached file (for master) #33677

DmytroAlipov · 2023-11-08T12:41:14Z

Description

The details are described in this discussion.

Now everything works correctly:

Do not pay attention to the fact that the screenshot was taken from a local instance. It has been tested on the production server, and everything works just as correctly.

openedx-webhooks · 2023-11-08T12:41:18Z

Thanks for the pull request, @DmytroAlipov! 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.

regisb · 2023-11-08T14:25:06Z

lms/djangoapps/ora_staff_grader/serializers.py

@@ -196,6 +197,22 @@ class UploadedFileSerializer(serializers.Serializer):
    name = serializers.CharField()
    size = serializers.IntegerField()

+    def to_representation(self, instance):


Would it be possible instead to change the serializer of downloadURL? I'm thinking something along the lines of: downloadUrl = AbsoluteURLField(source="download_url")

Not sure if my proposition is better, or even possible at all.

Do you mean something like this?

class AbsoluteURLField(serializers.URLField): def to_representation(self, value): if value and not value.startswith(("http://", "https://")): return f"{settings.LMS_ROOT_URL}{value}" return value class UploadedFileSerializer(serializers.Serializer): downloadUrl = AbsoluteURLField(source="download_url") ...

We could use the existing openedx.core.lib.api.fields.AbsoluteURLField but that requires passing in the view's request as context (see docs), and this isn't compatible with how UploadedFileSerializer is constructed.

So I think it's ok to keep this change as-is.

I wonder if you considered using SerializerMethodField instead of overriding to_representation. I tried this and it worked:

class UploadedFileSerializer(serializers.Serializer): """Serializer for a file uploaded as a part of a response""" downloadUrl = serializers.SerializerMethodField(method_name="get_download_url") description = serializers.CharField() name = serializers.CharField() size = serializers.IntegerField() def get_download_url(self, obj): """ Get the representation for SerializerMethodField `downloadUrl` """ return urljoin(settings.LMS_ROOT_URL, obj["download_url"])

So, I'm curious about why this is the chosen approach. I want to be sure I'm not missing something. Thanks!

dyudyunov · 2023-11-09T08:00:12Z

lms/djangoapps/ora_staff_grader/serializers.py

+            full_download_url = f"{settings.LMS_ROOT_URL}{download_url}"
+            data["downloadUrl"] = full_download_url


Suggested change

full_download_url = f"{settings.LMS_ROOT_URL}{download_url}"

data["downloadUrl"] = full_download_url

data["downloadUrl"] = f"{settings.LMS_ROOT_URL}{download_url}"

I think we could get rid of the full_download_url variable because it is used only once.

Also, consider using urljoin for the absolute URL construction:

data["downloadUrl"] = urljoin(settings.LMS_ROOT_URL, download_url)

But be careful with expected URL parts (processing different cases with leading/trailing slashes (/))

You can also use urljoin with two absolute URLs, so we could probably get rid of not download_url.startswith(("http://", "https://")) check. But I suspect that it will be less readable. You can check some examples here

I tried different options and they all work. I selected urljoin(settings.LMS_ROOT_URL, download_url). The nuances of use should not be a problem for us.

pomegranited

👍 @DmytroAlipov thank you for submitting this fix! It works perfectly.

I'm sorry it's taken so long to get reviewed. Unfortunately I'm not a core contributor for edx-platform, but I'll ping some people that are to see if we can get this and your backport PRs merged soon.

FYI @pkulkark @navinkarkera @farhaanbukhsh @MaferMazu @mariajgrimaldi

I tested this on my devstack by following the instructions to enable frontend-app-ora-grading and view files uploaded to an ORA problem.
I read through the code
I checked for accessibility issues
Includes documentation
User-facing strings are extracted for translation

pomegranited · 2024-01-02T00:57:01Z

lms/djangoapps/ora_staff_grader/serializers.py

@@ -196,6 +197,22 @@ class UploadedFileSerializer(serializers.Serializer):
    name = serializers.CharField()
    size = serializers.IntegerField()

+    def to_representation(self, instance):


We could use the existing openedx.core.lib.api.fields.AbsoluteURLField but that requires passing in the view's request as context (see docs), and this isn't compatible with how UploadedFileSerializer is constructed.

So I think it's ok to keep this change as-is.

pomegranited · 2024-01-07T22:14:50Z

FYI @mphilbrick211

mariajgrimaldi · 2024-01-22T13:42:50Z

I'll review this one once I have a chance. Thanks @pomegranited for the ping :). After merging this one, I'll review the backport PRs as part of the BTR. Thanks folks!

DmytroAlipov · 2024-01-29T10:31:09Z

I wonder if you considered using SerializerMethodField instead of overriding to_representation. I tried this and it worked:
class UploadedFileSerializer(serializers.Serializer):
    """Serializer for a file uploaded as a part of a response"""

    downloadUrl = serializers.SerializerMethodField(method_name="get_download_url")
    description = serializers.CharField()
    name = serializers.CharField()
    size = serializers.IntegerField()

    def get_download_url(self, obj):
        """
        Get the representation for SerializerMethodField `downloadUrl`
        """
        return urljoin(settings.LMS_ROOT_URL, obj["download_url"])
So, I'm curious about why this is the chosen approach. I want to be sure I'm not missing something. Thanks!

Hi @mariajgrimaldi!
Both options do not have very significant advantages over each other, neither in the context of memory nor in the context of execution time. My option using a URLField with a source argument may be slightly faster than the SerializerMethodField since it doesn't require an additional get_download_url method to be called.

But in practice, this difference will be so small that it is almost unnoticeable.
Therefore, it seems to me that the choice depends on our inner perfectionism :))

DmytroAlipov · 2024-01-29T11:12:45Z

@mariajgrimaldi
I will redo this shortly as you suggested.

farhaanbukhsh · 2024-01-31T06:56:29Z

Sorry @pomegranited I was off when you pinged. @mariajgrimaldi Thank you for reviewing it and let me know if I can help you with the review. :)

mariajgrimaldi · 2024-02-01T13:47:07Z

@DmytroAlipov: thanks for the detailed response! The implementation looks good to me; don't worry about changing it. Could you check the failing tests?

DmytroAlipov · 2024-02-01T15:05:41Z

@mariajgrimaldi I'm currently busy with other tasks, but I plan to deal with this PR early next week.

mariajgrimaldi · 2024-02-01T15:38:31Z

@DmytroAlipov: don't worry. Please let me know when it's ready so we can merge :)

After clicking the "View all responses" button in the ORA Staff Grading App, an error page appears for ORAs with file uploads.

DmytroAlipov · 2024-02-05T18:49:27Z

Hi @mariajgrimaldi!
Finally, I got to this PR🤓 I did as you suggested. Everything worked, but I had to slightly tweak a few old tests. When this PR is merged, I will update all backports and ping you!

openedx-webhooks · 2024-02-06T13:40:20Z

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

mariajgrimaldi · 2024-02-06T13:40:56Z

Thank you so much @DmytroAlipov!

edx-pipeline-bot · 2024-02-06T15:16:55Z

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

edx-pipeline-bot · 2024-02-06T15:34:39Z

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 Nov 8, 2023

DmytroAlipov force-pushed the fix-ora-uploaded-response branch from 10ac307 to 0232e82 Compare November 8, 2023 13:15

This was referenced Nov 8, 2023

fix: ORA response with attached file (for Quince) #33676

Merged

Fix ORA uploaded response (for Palm) #33678

Closed

regisb reviewed Nov 8, 2023

View reviewed changes

dyudyunov reviewed Nov 9, 2023

View reviewed changes

DmytroAlipov force-pushed the fix-ora-uploaded-response branch from 0232e82 to 8636958 Compare November 12, 2023 20:18

pomegranited approved these changes Jan 2, 2024

View reviewed changes

mariajgrimaldi self-requested a review January 22, 2024 13:43

mariajgrimaldi approved these changes Feb 5, 2024

View reviewed changes

DmytroAlipov force-pushed the fix-ora-uploaded-response branch from 0839d45 to 70fa522 Compare February 5, 2024 18:17

fix: ORA response with attached file (for master)

bb68e09

After clicking the "View all responses" button in the ORA Staff Grading App, an error page appears for ORAs with file uploads.

DmytroAlipov force-pushed the fix-ora-uploaded-response branch from 70fa522 to bb68e09 Compare February 5, 2024 18:28

mariajgrimaldi merged commit d65841a into openedx:master Feb 6, 2024
46 checks passed

DmytroAlipov deleted the fix-ora-uploaded-response branch February 6, 2024 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ORA response with attached file (for master) #33677

fix: ORA response with attached file (for master) #33677

DmytroAlipov commented Nov 8, 2023

openedx-webhooks commented Nov 8, 2023

regisb Nov 8, 2023

DmytroAlipov Nov 8, 2023

pomegranited Jan 2, 2024

mariajgrimaldi Jan 23, 2024 •

edited

Loading

dyudyunov Nov 9, 2023

DmytroAlipov Nov 13, 2023

pomegranited left a comment

pomegranited Jan 2, 2024

pomegranited commented Jan 7, 2024

mariajgrimaldi commented Jan 22, 2024 •

edited

Loading

DmytroAlipov commented Jan 29, 2024 •

edited

Loading

DmytroAlipov commented Jan 29, 2024

farhaanbukhsh commented Jan 31, 2024

mariajgrimaldi commented Feb 1, 2024 •

edited

Loading

DmytroAlipov commented Feb 1, 2024

mariajgrimaldi commented Feb 1, 2024

DmytroAlipov commented Feb 5, 2024 •

edited

Loading

openedx-webhooks commented Feb 6, 2024

mariajgrimaldi commented Feb 6, 2024

edx-pipeline-bot commented Feb 6, 2024

edx-pipeline-bot commented Feb 6, 2024

		full_download_url = f"{settings.LMS_ROOT_URL}{download_url}"
		data["downloadUrl"] = full_download_url

	full_download_url = f"{settings.LMS_ROOT_URL}{download_url}"
	data["downloadUrl"] = full_download_url
	data["downloadUrl"] = f"{settings.LMS_ROOT_URL}{download_url}"

fix: ORA response with attached file (for master) #33677

fix: ORA response with attached file (for master) #33677

Conversation

DmytroAlipov commented Nov 8, 2023

Description

openedx-webhooks commented Nov 8, 2023

regisb Nov 8, 2023

Choose a reason for hiding this comment

DmytroAlipov Nov 8, 2023

Choose a reason for hiding this comment

pomegranited Jan 2, 2024

Choose a reason for hiding this comment

mariajgrimaldi Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

dyudyunov Nov 9, 2023

Choose a reason for hiding this comment

DmytroAlipov Nov 13, 2023

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited Jan 2, 2024

Choose a reason for hiding this comment

pomegranited commented Jan 7, 2024

mariajgrimaldi commented Jan 22, 2024 • edited Loading

DmytroAlipov commented Jan 29, 2024 • edited Loading

DmytroAlipov commented Jan 29, 2024

farhaanbukhsh commented Jan 31, 2024

mariajgrimaldi commented Feb 1, 2024 • edited Loading

DmytroAlipov commented Feb 1, 2024

mariajgrimaldi commented Feb 1, 2024

DmytroAlipov commented Feb 5, 2024 • edited Loading

openedx-webhooks commented Feb 6, 2024

mariajgrimaldi commented Feb 6, 2024

edx-pipeline-bot commented Feb 6, 2024

edx-pipeline-bot commented Feb 6, 2024

mariajgrimaldi Jan 23, 2024 •

edited

Loading

mariajgrimaldi commented Jan 22, 2024 •

edited

Loading

DmytroAlipov commented Jan 29, 2024 •

edited

Loading

mariajgrimaldi commented Feb 1, 2024 •

edited

Loading

DmytroAlipov commented Feb 5, 2024 •

edited

Loading