-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: [FC-86] add org_image_url to course_discovery index FC-86 #36846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [FC-86] add org_image_url to course_discovery index FC-86 #36846
Conversation
|
Thanks for the pull request, @andrii-hantkovskyi! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
@diana-villalvazo-wgu, whenever you have some time, could you please take a look at this PR? |
|
@andrii-hantkovskyi for all PRs related to FC-86, please remember to put "FC-86" in the title of each pull request. |
cms/envs/common.py
Outdated
| # .. setting_default: organization_logos/_placeholder.png # FIXME: change to correct placeholder | ||
| # .. setting_description: The default logo URL for organizations that do not have a logo set. | ||
| # .. setting_warning: This URL is used as a placeholder for organizations that do not have a logo set. | ||
| DEFAULT_ORG_LOGO_URL = 'organization_logos/_placeholder.png' # FIXME: change to correct placeholder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should this FIXME be addressed? We shouldn't put FIXMEs in the code because it looks like this code is in an incomplete state. Is this placeholder logo being provided by the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the tip about adding that placeholder to the code, added :)
cms/envs/common.py
Outdated
| # .. setting_default: organization_logos/_placeholder.png # FIXME: change to correct placeholder | ||
| # .. setting_description: The default logo URL for organizations that do not have a logo set. | ||
| # .. setting_warning: This URL is used as a placeholder for organizations that do not have a logo set. | ||
| DEFAULT_ORG_LOGO_URL = 'organization_logos/_placeholder.png' # FIXME: change to correct placeholder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a piece of checked in asset that will always be there or will this be a broken link unless the operators do something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After Sarina's comment, I decided to add placeholder.png to this PR so that the link won't be broken
e756c1b to
66c03af
Compare
|
I would not use the edX logo as a placeholder logo. I believe we already have a placeholder image for a site logo, I'm not sure you need to make a new one. It's two overlapping circles and it's rendered in the header already. |
|
Removed placeholder, now refers to |
feanil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should there be a test for this or should the courseware API tests be updated to ensure this data is coming through correctly?
cms/envs/common.py
Outdated
| # .. setting_default: 'images/logo.png' | ||
| # .. setting_description: The default logo path for organizations that do not have a logo set. | ||
| # .. setting_warning: This path is used as a placeholder for organizations that do not have a logo set. | ||
| DEFAULT_ORG_LOGO_PATH = 'images/logo.png' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this only support being a path under the current STATIC_URL is a bit limiting. I think you should instead have the setting be the DEFAULT_ORG_LOGO_URL and accept any valid URLs here. By default it can be something like:
DEFAULT_ORG_LOGO_URL = Derived(lambda settings: settings.STATIC_URL + 'images/logo.png')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Add org_image_url to Course Discovery Index
This PR adds an
org_image_urlfield to the course discovery index to expose the organization's logo alongside course metadata.Key changes:
org_image_urlto the course index data viacourse_organization_image_url(course).course_organization_image_urlhelper, returning the organization's logo or a default placeholder if none is set.DEFAULT_ORG_LOGO_URLin settings as the fallback logo path.These changes enable clients to display organization branding in course listings, supporting improved visual context and a richer UI experience.
Unit and integration tests should verify correct selection of organization logos and fallback behavior when a logo is missing.