Skip to content

Added show_share_button column to newsletters#26939

Merged
troyciesco merged 1 commit intomainfrom
FEA-480_show-share-button-migration
Mar 25, 2026
Merged

Added show_share_button column to newsletters#26939
troyciesco merged 1 commit intomainfrom
FEA-480_show-share-button-migration

Conversation

@troyciesco
Copy link
Contributor

@troyciesco troyciesco commented Mar 24, 2026

ref https://linear.app/ghost/issue/FEA-480

  • adds the show_share_button column to newsletters to prep for future share button functionality
  • includes the bare minimum for setting up the migration and for tests to pass. In this case it includes updating snapshots related to the newsletter
  • also bumps to 6.23.0-rc.0

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6e6424a0-55d3-428e-8c6d-ff498fbceffa

📥 Commits

Reviewing files that changed from the base of the PR and between b07ca74 and dbbdc52.

⛔ Files ignored due to path filters (2)
  • ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap is excluded by !**/*.snap
  • ghost/core/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap is excluded by !**/*.snap
📒 Files selected for processing (6)
  • ghost/admin/package.json
  • ghost/core/core/server/data/migrations/versions/6.23/2026-03-24-15-55-52-add-show-share-button-to-newsletters.js
  • ghost/core/core/server/data/schema/schema.js
  • ghost/core/core/server/models/newsletter.js
  • ghost/core/package.json
  • ghost/core/test/unit/server/data/schema/integrity.test.js
✅ Files skipped from review due to trivial changes (5)
  • ghost/core/package.json
  • ghost/admin/package.json
  • ghost/core/test/unit/server/data/schema/integrity.test.js
  • ghost/core/core/server/data/migrations/versions/6.23/2026-03-24-15-55-52-add-show-share-button-to-newsletters.js
  • ghost/core/core/server/data/schema/schema.js

Walkthrough

This change introduces a new show_share_button field to the newsletter functionality. The version is bumped to 6.23.0-rc.0 in both the admin and core packages. A database migration adds the non-nullable boolean column to the newsletters table with a default value of false. The schema definition and Newsletter model are updated to include the new field with matching defaults. A schema integrity test hash is recalculated to reflect the database schema changes.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change—adding a show_share_button column to newsletters—which is the primary focus of the entire changeset.
Description check ✅ Passed The description is directly related to the changeset, providing context via issue reference, explaining the purpose of the column addition, and documenting the version bump and test updates included.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch FEA-480_show-share-button-migration

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Mar 24, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2026

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

General requirements

  • ⚠️ Tested performance on staging database servers, as performance on local machines is not comparable to a production environment
  • 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

@troyciesco troyciesco force-pushed the FEA-480_show-share-button-migration branch from bfa013c to e6aad7d Compare March 24, 2026 17:29
@github-actions
Copy link
Contributor

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 23503341707 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@troyciesco troyciesco force-pushed the FEA-480_show-share-button-migration branch 2 times, most recently from d43d63a to 642bd8c Compare March 24, 2026 18:54
@codecov
Copy link

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.23%. Comparing base (31e09b5) to head (dbbdc52).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26939      +/-   ##
==========================================
+ Coverage   73.18%   73.23%   +0.04%     
==========================================
  Files        1539     1539              
  Lines      121668   121669       +1     
  Branches    14718    14729      +11     
==========================================
+ Hits        89039    89100      +61     
+ Misses      31621    31540      -81     
- Partials     1008     1029      +21     
Flag Coverage Δ
admin-tests 54.38% <ø> (+0.01%) ⬆️
e2e-tests 73.23% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@troyciesco troyciesco marked this pull request as ready for review March 24, 2026 19:34
@troyciesco troyciesco force-pushed the FEA-480_show-share-button-migration branch from 642bd8c to b07ca74 Compare March 24, 2026 21:51
show_comment_cta: boolean;
show_subscription_details: boolean;
show_latest_posts: boolean;
show_share_button: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont think this type update and the newsletters.json normally get included in a migration PR...but i think they should?

When i first set up this PR i basically just had the bare min migration files, but migration-related changes in tests failed here.

So i added it to the model and it seemed to pass from there, and i had to then update the snapshots accordingly. with that in mind it makes sense to me that the type and the test response data also should be updated.

Unless i hit a weird sqlite edge-case on the first run in that test? i'm not sure, i'd never hit that error before when doing a migration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Genuinely just a question, I'm not sure: given that the admin pieces of this will deploy right away ahead of the migration, will this cause any issues if this field doesn't exist in the API responses?

Copy link
Collaborator

@cmraible cmraible Mar 25, 2026

Choose a reason for hiding this comment

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

Unless i hit a weird sqlite edge-case on the first run in that test? i'm not sure, i'd never hit that error before when doing a migration.

I've also never encountered that error, but it does seem a bit fishy. You've defined a default value, so it shouldn't encounter any NOT NULL constraints 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we've talked outside this thread about this, but to summarize here for the curious (cc @EvanHahn):

given that the admin pieces of this will deploy right away ahead of the migration, will this cause any issues if this field doesn't exist in the API responses?
we don't think so. once this is merged, these server changes won't deploy to Pro right away, they'll need a rollout. but because there are no changes to the admin code to try and reference/use show_share_button we should be ok, even if it's still hitting server version 6.22.1, effectively. once the server is next rolled out it will be 6.23-rc.0, and then we can safely add code that references it.

Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM other than @cmraible's question, which is a good one.


module.exports = createAddColumnMigration('newsletters', 'show_share_button', {
type: 'boolean',
nullable: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: love when a field is not nullable.

Copy link
Collaborator

@cmraible cmraible left a comment

Choose a reason for hiding this comment

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

LGTM!

@troyciesco troyciesco force-pushed the FEA-480_show-share-button-migration branch from b07ca74 to dbbdc52 Compare March 25, 2026 19:16
@github-actions
Copy link
Contributor

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 23559505758 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

1 similar comment
@github-actions
Copy link
Contributor

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 23559505758 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@troyciesco troyciesco merged commit a0bd354 into main Mar 25, 2026
61 of 69 checks passed
@troyciesco troyciesco deleted the FEA-480_show-share-button-migration branch March 25, 2026 19:58
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.

3 participants