Skip to content
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

Add API for social media tags #2391

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Conversation

svenseeberg
Copy link
Member

@svenseeberg svenseeberg commented Aug 7, 2023

Short description

Add a new API endpoint that returns a ready to use HTML snippet with social media meta tags. The resulting HTML code can be included with Nginx Server Side Includes into the index.html of the web app.

Proposed changes

  • Register path /api/social/REGION_SLUG/LANGUAGE_SLUG/PATH
  • Search for suitable page objects and return tags for the first found object
  • Provide fallback strings if no page is found.
  • Introduce new settings: platform name & social media preview image

Side effects

  • Does not really work for other types. The issue here is that we plan to pass the path directly from the URL in the web app. That means events would come in as /api/social/testumgebung/ar/events/sprachmentoring-treffen-2$2023-08-12. Not sure if we can find a good way to handle this, as we definitely will have namespace collissions. However, we could add a dedicated route for /api/social/REGION_SLUG/LANGUAGE_SLUG/{events,locations}/PATH etc. Feedback is welcome.
  • Tests are still missing.
  • The template is in the cms app templates directory, not in the api app.
  • We need special handling for the hallo region slug as this is hallo aschaffenburg with a dedicated app in the Integreat CMS.
  • We currently have no concept of front end user languages in our CMS. With more translation files we could support localized preview descriptions.

Resolved issues

Fixes: #2084

Additional context

This page should basically be replaced by the social endpoint -> https://git.tuerantuer.org/Integreat/social-media-meta-tags/src/branch/master/meta.php

A few helpful links to get a better understanding of the why and all the follow ups:


Pull Request Review Guidelines

@codeclimate
Copy link

codeclimate bot commented Aug 7, 2023

Code Climate has analyzed commit 5158344 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 98.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.3% (0.0% change).

View more on Code Climate.

@svenseeberg svenseeberg force-pushed the feature/socialmedia-headers branch 3 times, most recently from e622c68 to 0f49800 Compare August 7, 2023 17:33
@svenseeberg
Copy link
Member Author

@f1sh1918 @sarahsporck currently we mix meta tags that are statically encoded in the app index.html file and some tags that are fetched from our API. My question: do we want too keep that mixed approach or should we either load meta tags from the index.html or API? If we do the mixed approach we need to decide which tags we keep in the index.html and which ones we need to move into the CMS.

Currently, I moved the content of our PHP script into https://github.com/digitalfabrik/integreat-cms/pull/2391/files#diff-450ba7df606ea74a9a19ea6c0ea8388b3be650029959822d92382958f7fcf560R1-R10 .

@svenseeberg svenseeberg force-pushed the feature/socialmedia-headers branch 3 times, most recently from 300533c to d1809a4 Compare August 8, 2023 06:34
@steffenkleinle
Copy link
Member

Does not really work for other types. The issue here is that we plan to pass the path directly from the URL in the web app. That means events would come in as /api/social/testumgebung/ar/events/sprachmentoring-treffen-2$2023-08-12. Not sure if we can find a good way to handle this, as we definitely will have namespace collissions. However, we could add a dedicated route for /api/social/REGION_SLUG/LANGUAGE_SLUG/{events,locations}/PATH etc. Feedback is welcome.

I am pretty sure that the suffix $2023-08-12 is not the problem but if it is, we already have an issue to remove this again soon: digitalfabrik/integreat-app#2031
So if that would not work for some time, not a problem imo.

@sarahsporck
Copy link
Contributor

As far as I'm concerned I think setting social media headers in the backend as much as possible should be the preferred solution. In the best case scenario this would also include json ld tags.

Therefore, the following tags seem to be missing:

<meta property="og:site_name" content="integreat.app/malte.app/halloaschaffenburg.app">
<meta property="og:image" content="<%= config.icons.socialMediaPreview %>" />
<meta property="og:image:width" content="1200" />
<meta property="og:image:height" content="630" />
<meta property='og:url' content={window.location.href} /> // not sure about this one

And besides these there are the json+ld tags (events, breadcrumbs), which are a little bit more complicated, and adding these could also be discussed separately.

Any other tags that are calculated in the frontend should not have anything to do with how search engines or social media crawlers see our site.

@sarahsporck
Copy link
Contributor

@svenseeberg do you need more help or input for this? If you're too busy right now, I could take over :)

@svenseeberg
Copy link
Member Author

@svenseeberg do you need more help or input for this? If you're too busy right now, I could take over :)

Please feel always free to add new commits to my branches 😅 I think I can continue to work on this PR next week at the earliest.

@svenseeberg
Copy link
Member Author

svenseeberg commented Sep 23, 2023

We also have a problem with Aschaffenburg, because they are in the same CMS and I don't want the social media previews to be region specific. Well, we may have to create region settings with global fallbacks. Or maybe it is time to move Aschaffenburg into their own CMS.

@svenseeberg svenseeberg marked this pull request as ready for review September 23, 2023 15:39
@svenseeberg svenseeberg requested a review from a team as a code owner September 23, 2023 15:39
@svenseeberg svenseeberg force-pushed the feature/socialmedia-headers branch 6 times, most recently from bd93453 to 8302876 Compare September 23, 2023 16:20
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 💪

Since we're thinking about moving aschaffenburg into its own cms, this is probably not the final state anyways, but I just left my preliminary comments anyway 🙈

integreat_cms/core/settings.py Outdated Show resolved Hide resolved
integreat_cms/api/v3/socialmedia_headers.py Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/core/settings.py Outdated Show resolved Hide resolved
integreat_cms/api/v3/socialmedia_headers.py Outdated Show resolved Hide resolved
integreat_cms/release_notes/2023/unreleased/2391.yml Outdated Show resolved Hide resolved
integreat_cms/api/v3/socialmedia_headers.py Outdated Show resolved Hide resolved
integreat_cms/api/urls.py Outdated Show resolved Hide resolved
integreat_cms/api/v3/socialmedia_headers.py Outdated Show resolved Hide resolved
integreat_cms/api/v3/socialmedia_headers.py Outdated Show resolved Hide resolved
@svenseeberg svenseeberg force-pushed the feature/socialmedia-headers branch 4 times, most recently from f625972 to 6d579c1 Compare October 12, 2023 10:59
@david-venhoff david-venhoff force-pushed the feature/socialmedia-headers branch 2 times, most recently from 6c17c59 to 0b31fb4 Compare March 25, 2024 12:56
@david-venhoff david-venhoff dismissed timobrembeck’s stale review April 12, 2024 11:10

suggestions are implemented now

@david-venhoff david-venhoff requested review from DigitalfabrikMember and a team and removed request for DigitalfabrikMember April 12, 2024 11:10
@david-venhoff david-venhoff added the ‼️ prio: high Needs to be resolved ASAP. label May 25, 2024
Copy link
Member Author

@svenseeberg svenseeberg left a comment

Choose a reason for hiding this comment

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

I just noted that the output is not yet exactly what we need. I have not invenstigated the reason.

The html and head tags should not be there.

<html lang="de-DE">
    <head>
        <title>Alltag - Augsburg | Integreat</title>
        <meta name="apple-mobile-web-app-title" content="Alltag - Augsburg | Integreat">
        <meta name="twitter:title " content="Alltag - Augsburg | Integreat">
        <meta name="twitter:card" content="summary">
        <meta property="og:title" content="Alltag - Augsburg | Integreat">
        
            <meta property="og:description" content="Gültiger LinkUngeprüfter LinkUngültiger LinkIgnorierter LinkInterner LinkFehlerhafter interner LinkE">
            <meta name="twitter:description" content="Gültiger LinkUngeprüfter LinkUngültiger LinkIgnorierter LinkInterner LinkFehlerhafter interner LinkE">
        
        <meta property="og:image" content="http://localhost:8000/static/logos/integreat/social-media-preview.png" />
        <meta property="og:image:width" content="1200" />
        <meta property="og:image:height" content="630" />
        <meta property="og:url" content="https://integreat.app/augsburg/de/alltag/" />

By default a HttpResponse should return an empty document. So its probably something that we do.

@timobrembeck
Copy link
Member

timobrembeck commented May 25, 2024

@svenseeberg the partial html response was my idea to be able to set the language tag of the html tag.
If we adapt the including template accordingly, it should work fine. Only the final result needs to be valid html, the preliminary header is just raw text and does not need to be parsed.
For more context about this, see #2391 (comment).

timobrembeck added a commit to digitalfabrik/integreat-app that referenced this pull request Jun 1, 2024
@timobrembeck timobrembeck force-pushed the feature/socialmedia-headers branch 3 times, most recently from 32db6da to b38c6e7 Compare June 1, 2024 16:31
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Finally, I think this is ready to be merged! 🎉

I opened a PR on the web app's side to adapt their part of the including template as well:

digitalfabrik/integreat-app#2809

Apart from that, I made some further changes to ensure that the response is also a partial HTML response when a 404 error occurs and I changed the tests to compare the literal html output instead of converting the API result to json first.

@david-venhoff could you check again whether everything makes sense at it is now? Thanks! 👍

And @svenseeberg do you have any concerns about this idea with the partial HTML response?

Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

🎉
This looks good to me. I found one question, but I think this pr is already in a good state and should not be delayed much further.

integreat_cms/api/v3/social_media_headers.py Outdated Show resolved Hide resolved
Co-authored-by: David Venhoff <[email protected]>
Co-authored-by: Timo Brembeck <[email protected]>
Co-authored-by: Sven Seeberg <[email protected]>
timobrembeck added a commit to digitalfabrik/integreat-app that referenced this pull request Jun 5, 2024
@timobrembeck timobrembeck merged commit 0e61a90 into develop Jun 5, 2024
5 checks passed
@timobrembeck timobrembeck deleted the feature/socialmedia-headers branch June 5, 2024 16:58
@svenseeberg
Copy link
Member Author

And @svenseeberg do you have any concerns about this idea with the partial HTML response?

No, should be okay 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
‼️ prio: high Needs to be resolved ASAP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Add social media headers endpoint
5 participants