fix: share urls in course about page#30389
Conversation
openedx-webhooks
added
needs triage
open-source-contribution
|
Thanks for the pull request, @ghassanmas! I've created OSPR-6680 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 as you can:
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. |
|
This ready for review, hopefully should be resolved by nutmeg |
|
@jmbowman This is ready for your team's review. |
rgraber
left a comment
rgraber
left a comment
There was a problem hiding this comment.
Hello! No major content issues, but a few questions and please fix the whitespace changes.
There was a problem hiding this comment.
I removed because its confusing or lets say unusual to have the ':' char after mentioning an account on twitter.
Before applying this change the generated tweet would like:
I just enrolled ..etc through @platformtwitteraccount: https..etc and after it's I just enrolled ..etc through @platformtwitteraccount https..etc
There was a problem hiding this comment.
@rgraber is it okay now to merge, or do you still have notes? Since nutmeg is going to be released in a couple of days
There was a problem hiding this comment.
where is this value passed in? can it be removed completely from there?
There was a problem hiding this comment.
It's passed as a href for for that tag below
There was a problem hiding this comment.
sorry, confusing comment. what i meant was can we delete course_about_facebook_link from wherever it originates now? or is it used elsewhre?
There was a problem hiding this comment.
I don't think its defined somewhere... It tries to get it first from site configuration if its enabled, and (if not or if its not defined in site configuration) it will then cascade/default to settings.PLATFORM_FACEBOOK_ACCOUNT.
There was a problem hiding this comment.
It takes the get_value function from static
https://github.com/openedx/edx-platform/blob/151bd136667d27bf85256e382ed5b5ea0144e00f/common/djangoapps/pipeline_mako/templates/static_content.html#L14-L17
which import the get_value from site_configuration https://github.com/openedx/edx-platform/blob/677e0b8076d3e75def14fe25fe0c66c6a7788748/openedx/core/djangoapps/site_configuration/helpers.py#L119-L135
ghassanmas
force-pushed
the
sharelinks-course-about
branch
2 times, most recently
from
c350cbf to
049691f
Compare
This commit does few changes fixes for the course_about_sidebar_template - It fixes incorrect share url ref: openedx/wg-build-test-release/issues/174 - It changes the icon for facebook instead of thumb up to faceboook logo - It remove redundancy in getting course about page url. squash
ghassanmas
force-pushed
the
sharelinks-course-about
branch
from
049691f to
8036ee7
Compare
|
Thank you so much @ghassanmas and all reviewers. This has been an issue in Open edX for, dare I say, over a year. |
|
@ghassanmas 🎉 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 Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
EdX Release Notice: This PR has been rolled back from the production environment. |
1 similar comment
|
EdX Release Notice: This PR has been rolled back from the production environment. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
|
EdX Release Notice: This PR has been deployed to the production environment. |
fix: share urls in course about page (#30389)
6 participants
Description
This commit does few changes fixes for the course_about_sidebar_template
Supporting information
Go to course about page and share via facebook.
Before it would link to https://www.facebook.com/YourPlatformFacebookAccount which event if the settings platform account is set, it wouldn't make a shareable expirence, add to that it assume that in order to share to facebook, the platform or the course must have a facebook account.
After applying this change it would use the official way of creating a fasebook share url:
https://www.facebook.com/sharer/sharer.php?u=course_url
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
Before Nutmeg release, (need to be cherry-picked to nutmeg once approaved/merged).