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

Implemented Ghost site changed webhook handler #5

Merged

Conversation

mike182uk
Copy link
Member

refs MOM-300

Implemented Ghost site changed webhook handler so that when the settings for a Ghost instance are updated, the actor data is also updated in the database (name, description, icon). For this to work, the Ghost instance needs to setup a webhook for the Site Changed event and point it to the <host>/.ghost/activitypub/webhooks/site/changed endpoint

Create,
Note,
Activity,
} from '@fedify/fedify';
import { v4 as uuidv4 } from 'uuid';
import { addToList } from './kv-helpers';
import { ContextData } from './app';

type PersonData = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to make use of this type in the webhook handler, but felt weird importing it from dispatchers so extracted "user" functionality to a user.ts module. Not sure what convention we want to use for locating shared types etc.

refs [MOM-300](https://linear.app/tryghost/issue/MOM-300/update-actor-data-with-ghost-site-settings#comment-4e49803f)

Implemented Ghost site changed webhook handler so that when the settings for a
Ghost instance are updated, the actor data is also updated in the database
(name, description, icon). For this to work, the Ghost instance needs to setup
a webhook for the `Site Changed` event and point it to the
`<host>/.ghost/activitypub/webhooks/site/changed` endpoint
@mike182uk mike182uk force-pushed the mike-mom-300-update-actor-data-with-ghost-site-settings branch from b3c404f to f002146 Compare July 24, 2024 15:36
src/handlers.ts Outdated
Comment on lines 139 to 144
// Retrieve site settings from Ghost
const host = ctx.req.header('host');

const settings = await ky
.get(`https://${host}/ghost/api/admin/site/`)
.json<GhostSiteSettings>();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is sus, but not sure what the best thing to do here is as we don't know anything about the requesting client (so am just assuming that the request came from the Ghost instance on that is accessible on the same host 🙃)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, agree this is sus! Maybe something like:

async function getGhostSiteSettings(host: string): Promise<GhostSiteSettings> {
  const url = new URL(`https://${host}/ghost/api/admin/site/`);
  const settings = await ky
            .get(`https://${host}/ghost/api/admin/site/`)
            .json<unknown>();
  
  // Some assertions here so TS doesn't complain about `unknown`
  
  return {
    site: {
      description: settings?.site?.description || 'This is a summary',
      title: settings?.site?.title || 'index',
      icon: settings?.site?.icon || 'https://ghost.org/favicon.ico',
    }
  };
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The above couples site settings -> default actor data, which isn't great, but it's a step forward. Would be better to have null as the default instead and then default in the handler probs

Copy link
Member Author

Choose a reason for hiding this comment

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

For me the sus part was using the host header and assuming thats where the original request was coming from in order to make the request to get the site settings 😄

But good to know you think the rest is sus 😂

Is there a need for the defaults? The retrieved settings are merged with the existing data - If any of the expected settings are not present or null, the existing data would be used?

🤔 I liked the typing of the response, but understand that that might not actually be what we get, was optimising for the happy path 😄 We could add a validation step? (i.e isSiteSettings()) or do you think its not worth the hassle over optional chaining?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think typing the response is nice - but we don't actually know what we get, so I was thinking that the lil wrapper gives us the type AND verifies it

RE: The host header - that's all good IMO, that's how we're differentiating between different sites

Defaults - hmm, I think we should have some defaults - because we don't want empty profiles, I don't think the object spread operator will ignore null values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeh ofc the null values wont get dropped 🤦‍♂️ Have gone with this approach and added in the defaults, nice one

@mike182uk mike182uk force-pushed the mike-mom-300-update-actor-data-with-ghost-site-settings branch from 4b0ea75 to 640e568 Compare July 25, 2024 13:08
@mike182uk mike182uk merged commit c8df524 into main Jul 25, 2024
1 check passed
@mike182uk mike182uk deleted the mike-mom-300-update-actor-data-with-ghost-site-settings branch July 25, 2024 13:56
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