[SE-3329] Expose banner image url in course API

[gabor-boros]

This PR extends the course overview and detail APIs to include (both relative and absolute) banner image url, which allows content publishers to specify a banner image and then reuse that from a custom UI/integration.

Dependencies: None

Screenshots:

Sandbox URL: https://pr25045.sandbox.opencraft.hosting/

Merge deadline: Preferably 29th of September

Testing instructions:

1. Open edX Studio from Content -> Files & Uploads, then copy the file name
2. Paste the file name into Course Banner Image field into Settings -> Advanced Settings page
3. Open <URL>/api/courses/v1/courses/ to check all the courses, where URL is the sandbox LMS URL
4. Open <URL>/api/courses/v1/courses/<course_id> to see all the courses, where URL is the sandbox LMS URL and course_id is the course id

Author notes and concerns:

1. The _AbsolutMediaSerializer is using a private attribute from AbsoluteURLField, but I found no other way to avoid code duplication and have the same absolute url conversion functionality, since AbsoluteURLField requires the request object
   Currently looking for ways to fix it.

Reviewers

• @toxinu
• @bradenmacdonald

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

[openedx-webhooks]

Thanks for the pull request, @gabor-boros! I've created OSPR-4994 to keep track of it in JIRA, where we prioritize reviews. 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:

• 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.

[toxinu]

@gabor-boros Code reviewed and I go through testing instructions. Please ping me when you want another review 👍🏻

[gabor-boros]

Then I'm pinging you @toxinu 😊

[natabene]

@gabor-boros Thank you for your contribution. Please let me know once this is ready for our review.

[toxinu]

@gabor-boros @natabene Reviewed and approved 👍🏻

[gabor-boros]

@natabene It is ready from our side 😊

[gabor-boros]

@natabene do I need to do anything else related to this PR? ☺️

[natabene]

@gabor-boros No, just wait till we can get to it.

[bradenmacdonald]

@gabor-boros Overall seems good, but I have a few requested cleanups.

[bradenmacdonald]

I believe this hack is required because you're declaring the field within this method, instead of as a member of this serializer.

I think that if you just declare the field like this:

class _AbsolutMediaSerializer(_MediaSerializer):
    requires_context = True
    url = AbsoluteURLField(source="get_url")

    def get_url(self):
        ...

then it should work without context hacks.

Edit: Hmm, never mind I guess that won't work because it won't call the method on the serializer. Maybe you can override __init__ to get data and call apply_cdn_to_url on the data there? I think that's cleaner than overriding __call__ and passing the context around like this.

[gabor-boros]

Unfortunately, this cannot be done, because the BaseSerializer does not set the self.initial_data if data argument was empty. Although serializers has a get_initial method, the method will return an empty OrderedDict since the serializer did not receive data at all. So we need to pass the context around, but I'm opened to other ideas as well. When I did a research in this topic when I opened the PR, I did not find any other solution which is cleaner.

[bradenmacdonald]

This test has the same docstring as the previous test and it's hard to tell the difference. It took me a while to figure out that this is testing a slightly different method. Plus it doesn't actually test what it says in the docstring, because this test case doesn't test the case where start_url is already an absolute URL.

Maybe combine the tests, or change the docstring to say "Test the apply_cdn_to_url method directly when used with a single URL"

[gabor-boros]

Oh, it seems I overlooked the docstring here. Thanks for pointing out! I'll fix this as well

[bradenmacdonald]

I think you forgot to address this review comment ^

[gabor-boros]

Oh, you are totally right. I'm doing it just right now. Sorry about that.

[gabor-boros]

@bradenmacdonald I just adjusted most of the comments and updated the images in the description. Could you please give it another review round?

[bradenmacdonald]

Thanks @gabor-boros. Looking good now. Please address the remaining issue with the test/docstring, get the tests passing, and squash down to a single commit with a useful commit message. I will then approve.

Also, can you link me directly to an example on the sandbox where I can see the output? And in future, please use sandboxes from Ocim prod, not Ocim stage, since the stage ones don't have valid HTTPS certificates.

[gabor-boros]

Thank you for the review @bradenmacdonald!

I did adjust the comments, squashed the commits and replaced the sandbox URL in the PR description besides made sure that tests are passing. Also I realized I made a typo in the migration file name so it is fixed as well.

PS: the appserver is still provisioning

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[bradenmacdonald]

👍

• I tested this: at https://pr25045.sandbox.opencraft.hosting/api/courses/v1/courses/
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: docstrings etc. yes

[openedx-webhooks]

@gabor-boros 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[nasthagiri]

Thank you for this platform enhancement.
Where may I find documentation on how a "banner image" differs from a "course image"?

[bradenmacdonald]

Where may I find documentation on how a "banner image" differs from a "course image"?

@nasthagiri I see some context here, but not sure that it qualifies as "documentation" : https://github.com/edx/edx-platform/blob/7afee25ce2150443eefe0ae60b81f3e4f6e61900/common/lib/xmodule/xmodule/course_module.py#L589-L609

[nasthagiri]

From a consumer perspective, it would be useful to know when the difference between the 2 images. Perhaps it would help to describe where each image is displayed?

[bradenmacdonald]

ping @gabor-boros - can you answer that question ^ ?

[gabor-boros]

@nasthagiri & @bradenmacdonald Apologies for replying that late, I probably missed the notification about this comment. As far as I can see, the banner_image is part of the extended_course_details. The original addition was part of edx@f7f281b as far as I can see, but I cannot find more information about that either.

From consumer perspective - in my mind - the difference is that the course image is kind of a thumbnail of a course while the banner image is like a cover image. So something like this:

[edx-pipeline-bot]

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

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.