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

feat(preview): added preview images #848

Merged
merged 7 commits into from
Jul 27, 2023
Merged

feat(preview): added preview images #848

merged 7 commits into from
Jul 27, 2023

Conversation

caando
Copy link
Contributor

@caando caando commented Jul 20, 2023

Problem

CMS dashboard page displays Isomer logo for all sites

Closes IS-266

Solution

This PR implements new POST backend endpoint that takes in a list of site names and try to get urls of preview images for those site. It tries to get the favicon from production site as preview image by parsing the HTML from index page, using a GET request to the url of the production site. If it is unsuccessful, undefined will be returned.

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible

Features:

  • new /sites/preview/ POST endpoint that returns the image urls for the favicons of the requested sites.

Before & After Screenshots

BEFORE:

image

AFTER:

image

Tests

Visual test by visiting the dashboard of all sites.

New dependencies:

  • dependency : jsdom, which is previously already a dependancy of the backend. This PR adds @types/jsdom to package.json.

Note

Requires isomerpages/isomercms-frontend#1360 to be merged in.

The /sites/preview is a POST endpoint as the frontend sends a list of sites to obtain previews for. This is to support GitHub login users who we don't have the list of sites for users in the db. However, using GET endpoint without sending a list of sites is ideal for caching of responses. Should all users be migrated to email based login in the future, a db query with session data can be used to obtain list of sites and endpoint can be changed to GET.

We also deny this feature to admin users or request with more than limit (50) number of sites as they are most likely made by admin Github users. This is because fetching previews is expensive and incur high latency. As admin users have hundreds of sites, this results in high load for the system.

Todo

Add tests

@caando caando requested a review from a team July 20, 2023 20:25
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

may I confrim that the API design here is such that

if site's favicon does not exist, we still return a 200 {imageUrl : undefined}? I remember you mentioned that we timeout here, but the design of this pr seems to be what I would have requested changes for haha

src/services/identity/PreviewService.ts Outdated Show resolved Hide resolved
src/services/utilServices/Sanitizer.ts Outdated Show resolved Hide resolved
src/services/identity/SitesService.ts Outdated Show resolved Hide resolved
src/services/identity/SitesService.ts Outdated Show resolved Hide resolved
@caando
Copy link
Contributor Author

caando commented Jul 26, 2023

@kishore03109 Thanks for reviewing my PRs 💯

@caando caando requested review from kishore03109 and a team July 26, 2023 03:40
@kishore03109
Copy link
Contributor

kishore03109 commented Jul 26, 2023

@caando Bump on this >
may I confrim that the API design here is such that

if site's favicon does not exist, we still return a 200 {imageUrl : undefined}? I remember you mentioned that we timeout here, but the design of this pr seems to be what I would have requested changes for haha

TLDR; we shouldnt be

  1. timing out out requests
  2. returning 4xx errors for undefined images. we have alarms for 4xx, and we dont expect all sites to have a favicon either, would rather have a 2xx with the fallback image

@caando caando merged commit 32df557 into develop Jul 27, 2023
@mergify mergify bot deleted the feat/preview-images branch July 27, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants