Skip to content

Sitewide socials datamodel #23812

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cathysarisky
Copy link
Member

@cathysarisky cathysarisky commented Jun 12, 2025

This PR adds seven new columns to the settings data model, to hold seven new sitewide socials. I'm using the same social platforms as chosen for #22877 for consistency.

I've opted for empty strings rather than Ghost's socials for these new settings, because I've observed new Ghost users being confused by having Ghost's socials present, and not understanding how to remove unwanted social icons. It seems preferable to leave these blank and let the user discover them in due time.

  • I've read and followed the Contributor Guide
  • I've explained my change
  • I've written an automated test to prove my change works

Additional feature-flagged PRs to update the settings cache, update the social media entries in the admin app, and to ensure that these are available to handlebars in the @site object will follow.

Copy link
Contributor

coderabbitai bot commented Jun 12, 2025

Walkthrough

This change introduces seven new site-wide settings for various social media and content platforms: "threads," "bluesky," "mastodon," "tiktok," "youtube," "instagram," and "linkedin." The migration script adds these settings atomically, initializing each as a string with an empty default value. Corresponding updates are made to the site settings schema, the public settings cache typedef, and the allowlist for public settings. Unit tests are updated to reflect the increased number of settings, including adjustments to the expected key count and the integrity hash for the default settings.

Possibly related PRs

  • Added new social media columns to users table #22877: Adds new columns for the same social media platforms to the users table schema and migration; both PRs expand schema support for these platforms but target different tables (site-wide settings vs. user-specific data).
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

It looks like this PR contains a migration 👀
Here's the checklist for reviewing migrations:

General requirements

  • ⚠️ Tested on the staging database servers
  • Satisfies idempotency requirement (both up() and down())
  • Does not reference models
  • Filename is in the correct format (and correctly ordered)
  • Targets the next minor version
  • All code paths have appropriate log messages
  • Uses the correct utils
  • Contains a minimal changeset
  • Does not mix DDL/DML operations
  • Tested in MySQL and SQLite

Schema changes

  • Both schema change and related migration have been implemented
  • For index changes: has been performance tested for large tables
  • For new tables/columns: fields use the appropriate predefined field lengths
  • For new tables/columns: field names follow the appropriate conventions
  • Does not drop a non-alpha table outside of a major version

Data changes

  • Mass updates/inserts are batched appropriately
  • Does not loop over large tables/datasets
  • Defends against missing or invalid data
  • For settings updates: follows the appropriate guidelines

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Jun 12, 2025
@cathysarisky cathysarisky marked this pull request as ready for review June 12, 2025 03:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (4)
ghost/core/test/unit/server/data/exporter/index.test.js (1)

237-241: Hard-coding allowedKeysLength is brittle – consider deriving it

Every new public setting requires manual edits in two places (default-settings + this magic number).
A small helper would keep the assertion but remove the maintenance hotspot:

-const allowedKeysLength = 97;
+const allowedKeysLength = Object.keys(defaultSettings).reduce((acc, curr) => {
+    return acc + Object.keys(defaultSettings[curr]).length;
+}, 0) - SETTING_KEYS_BLOCKLIST.length;

The comment above the check still serves as a reminder, but the test no longer fails on purely additive, non-blocking keys.

ghost/core/core/shared/settings-cache/public.js (1)

14-20: Minor ordering / grouping nit

The newly-added social handles break the roughly alphabetical grouping of this allow-list.
Re-ordering for readability (e.g. keep all socials together: bluesky, facebook, instagram, linkedin, mastodon, threads, tiktok, twitter, youtube) would make future diffs easier to scan.
No functional impact; feel free to skip if list ordering is considered legacy-locked.

ghost/core/core/server/data/migrations/versions/5.124/2025-06-11-20-00-00-add-sitewide-data.js (1)

1-45: Reduce duplication with an array-driven helper

Seven near-identical addSetting calls make the migration verbose.
A compact loop is easier to scan and less error-prone:

-const {combineTransactionalMigrations, addSetting} = require('../../utils');
+const {combineTransactionalMigrations, addSetting} = require('../../utils');

-module.exports = combineTransactionalMigrations(
-    addSetting({key: 'threads', value: '', type: 'string', group: 'site'}),
-    addSetting({key: 'bluesky', value: '', type: 'string', group: 'site'}),
-    ...
-    addSetting({key: 'linkedin', value: '', type: 'string', group: 'site'})
-);
+const socials = ['threads', 'bluesky', 'mastodon', 'tiktok', 'youtube', 'instagram', 'linkedin'];
+
+module.exports = combineTransactionalMigrations(
+    ...socials.map(key => addSetting({key, value: '', type: 'string', group: 'site'}))
+);

Same behaviour, half the lines.

ghost/core/core/server/data/schema/default-settings/default-settings.json (1)

144-171: Consider enforcing URL or handle validation
Currently these settings accept any string. To prevent misconfiguration, you might add a validation rule (e.g. "isURL": true or a regex pattern) per key.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2469030 and 62cae09.

⛔ Files ignored due to path filters (1)
  • ghost/core/test/e2e-api/content/__snapshots__/settings.test.js.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • ghost/core/core/server/data/migrations/versions/5.124/2025-06-11-20-00-00-add-sitewide-data.js (1 hunks)
  • ghost/core/core/server/data/schema/default-settings/default-settings.json (1 hunks)
  • ghost/core/core/shared/settings-cache/CacheManager.js (1 hunks)
  • ghost/core/core/shared/settings-cache/public.js (1 hunks)
  • ghost/core/test/unit/server/data/exporter/index.test.js (1 hunks)
  • ghost/core/test/unit/server/data/schema/integrity.test.js (1 hunks)
🔇 Additional comments (3)
ghost/core/test/unit/server/data/schema/integrity.test.js (1)

38-41: Ensure accompanying DB version bump or migration reference is present

currentSettingsHash change reflects an altered default-settings payload (7 new socials keys).
Please double-check that:

  1. The settings table migration (2025-06-11-20-00-00-add-sitewide-data.js) is included in the DB version map so that Ghost knows the DB is ahead of earlier builds.
  2. knex-migrator migrate on a fresh DB produces the new hash (sanity check).

If both are already covered, feel free to ignore.

ghost/core/core/shared/settings-cache/CacheManager.js (1)

25-32: Docs in sync – nice catch

Typedef updated to include all seven new socials; keeps editor intellisense happy.

ghost/core/core/server/data/schema/default-settings/default-settings.json (1)

144-171: New site-wide social settings integrated
The seven new string-typed keys (threads, bluesky, mastodon, tiktok, youtube, instagram, linkedin) have been added under "site" with empty defaults, matching the existing pattern for social handles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration [pull request] Includes migration for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant