Skip to content

fix: share certificate in FB#31578

Merged
justinhynes merged 5 commits intoopenedx:masterfrom
DmytroAlipov:fix-FB-share
Sep 8, 2023
Merged

fix: share certificate in FB#31578
justinhynes merged 5 commits intoopenedx:masterfrom
DmytroAlipov:fix-FB-share

Conversation

@DmytroAlipov
Copy link
Contributor

Fixed an issue where the Course Card Image was missing in an FB post after a user shared it.
vanilla-1

Description

In order to share the certificate on your Facebook page, the code was used
FaceBook.share({share_text: '${facebook_share_text | n, js_escaped_string}', share_link: '${share_url | n, js_escaped_string}', picture_link: '${full_course_image_url | n, js_escaped_string}', description: '${_('Click the link to see my certificate.') | n, js_escaped_string}'});
In this case, the picture that was transmit in the picture_link parameter was ignored.

During the investigation, it was revealed that Facebook changed the rules for sharing. Meta tags must be used (follow FB rules).
After making changes to the code, the Course Card Image is displayed when you share the certificate.
In Studio:
vanilla-4
In FB:
vanilla-5

Sharing Debugger was used for debugging.
The default image is /static/studio/images/pencils.jpg if the course image is not loaded. Displayed as:
vanilla-6

NOTE: og:url and share_link are the same. The "share to FB" button will not work without share_link: '${share_url | n, js_escaped_string}'. But if you do not specify og:url, the FB debugger asks you to specify the entire set of meta tags. If you do not pass og:url there will be no errors, but I decided to do as FB recommends.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 17, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 17, 2023

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.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 20, 2023
@e0d e0d added product review PR requires product review before merging waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Jan 20, 2023
@e0d
Copy link
Contributor

e0d commented Jan 20, 2023

@DmytroAlipov looks like there's a failed test.

@DmytroAlipov
Copy link
Contributor Author

@e0d Hi! I found cases that led to incorrect work. Fixed everything and added tests. Please try again to run the test.

@e0d
Copy link
Contributor

e0d commented Jan 25, 2023

@DmytroAlipov I've approved the tests.

Fixed the Course Card Image is absent on FB post after sharing by user.
@DmytroAlipov
Copy link
Contributor Author

@e0d sorry, I used the f-string in the test where it was not needed, and the test failed in that place. I fixed it and push again.

@e0d
Copy link
Contributor

e0d commented Jan 25, 2023

@DmytroAlipov tests approved.

@mphilbrick211
Copy link

Hi @DmytroAlipov! Looks like this is ready for review/merge - can you confirm and I'll have someone take a look?

@mphilbrick211 mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 7, 2023
@DmytroAlipov
Copy link
Contributor Author

@mphilbrick211 Yes, sure. I confirm.

@mphilbrick211
Copy link

@jmakowski1123 @ProductRyan This is ready for your review.

@jmakowski1123 jmakowski1123 removed the product review PR requires product review before merging label Feb 22, 2023
@jmakowski1123
Copy link

jmakowski1123 commented Feb 22, 2023

On closer review, this is a simple bug fix and doesn't require product review. @mphilbrick211

@mphilbrick211
Copy link

Hi @hurtstotouchfire - could you take a look at this? Thanks!

@mphilbrick211
Copy link

Hi @DmytroAlipov - looks like some tests above need to be re-run. Would you mind taking a look?

@DmytroAlipov
Copy link
Contributor Author

Hi @mphilbrick211! So if the branch is up-to-date and the tests are passing, I'm not quite sure what I should do.

@mphilbrick211
Copy link

@DmytroAlipov looks good now! I've marked it as ready for review.

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jun 21, 2023
@e0d
Copy link
Contributor

e0d commented Jun 28, 2023

@hurtstotouchfire do you know when this review is scheduled for?

@DmytroAlipov
Copy link
Contributor Author

@e0d Hi! This PR is still not merged. Could you please ping the person responsible for continuing the review of this PR?

@mphilbrick211
Copy link

Hi @hurtstotouchfire and @openedx/2u-aperture! Just checking back in to see if this can get this reviewed soon?

@hurtstotouchfire hurtstotouchfire added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Aug 30, 2023
@justinhynes
Copy link
Contributor

I'm taking a look at this today! Sorry for the delay.

Comment on lines +19 to +20
## OG (Open Graph) url, title, type, image and description added below to give social media info to display
## (https://developers.facebook.com/docs/opengraph/howtos/maximizing-distribution-media-content#tags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a link back to the best practices! 👏

Copy link
Contributor

@justinhynes justinhynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update and all the extra context in the PR itself.

@e0d
Copy link
Contributor

e0d commented Sep 7, 2023

@justinhynes thanks for the review. @DmytroAlipov won't be able to merge, can you push the button when it works on your side?

@justinhynes
Copy link
Contributor

Ah, gotcha! I'll merge first thing tomorrow morning (USA -- EST time).

cc: @DmytroAlipov

@e0d
Copy link
Contributor

e0d commented Sep 7, 2023

I've merge changes on the base into the branch from the GitHub UI. This will cause the tests to re-run, but all should be GTG tomorrow.

@justinhynes justinhynes merged commit ad8ed53 into openedx:master Sep 8, 2023
@openedx-webhooks
Copy link

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

@DmytroAlipov DmytroAlipov deleted the fix-FB-share branch September 8, 2023 11:46
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs maintainer attention Issue or PR specifically needs the attention of the maintainer. open-source-contribution PR author is not from Axim or 2U waiting for eng review PR is ready for review. Review and merge it, or suggest changes.

Projects

Status: Done
Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants