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

Added migration and schema change for posts_meta.hide_title_and_feature_image #17187

Conversation

kevinansfield
Copy link
Contributor

@kevinansfield kevinansfield commented Jul 3, 2023

closes https://github.com/TryGhost/Team/issues/3550

We want to allow an option to hide the title and feature image on a per-page basis, to do that we need somewhere to store the setting value. The existing posts_meta table is the simplest candidate, especially as this is a single setting and we don't have a desire to introduce many such settings.

  • added migration that adds the hide_title_and_feature_image column to the posts_meta table with a boolean data type and a default value of false (matches behaviour of all existing pages)
  • updated schema file for initial database creation
  • removed property from API output via serializers to keep migration PR minimal

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Jul 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

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

General requirements

  • 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

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

@kevinansfield kevinansfield force-pushed the add-hide-title-and-feature-image-property branch 2 times, most recently from 40996cd to 5740a12 Compare July 3, 2023 17:17
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 🎉

Comparison is base (74f2e08) 71.70% compared to head (5740a12) 71.76%.

❗ Current head 5740a12 differs from pull request most recent head ceae2e2. Consider uploading reports for the commit ceae2e2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17187      +/-   ##
==========================================
+ Coverage   71.70%   71.76%   +0.05%     
==========================================
  Files        1836     1836              
  Lines      116454   116462       +8     
  Branches    17275    17298      +23     
==========================================
+ Hits        83508    83579      +71     
+ Misses      31829    31762      -67     
- Partials     1117     1121       +4     
Flag Coverage Δ
e2e-tests 66.52% <100.00%> (+0.03%) ⬆️
unit-tests 63.71% <55.55%> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
...ndpoints/utils/serializers/output/mappers/pages.js 100.00% <100.00%> (ø)
.../endpoints/utils/serializers/output/utils/clean.js 65.45% <100.00%> (+0.85%) ⬆️
ghost/core/core/server/models/posts-meta.js 83.72% <100.00%> (+0.38%) ⬆️

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…ure_image`

closes TryGhost/Product#3550

We want to allow an option to hide the title and feature image on a per-page basis, to do that we need somewhere to store the setting value. The existing `posts_meta` table is the simplest candidate, especially as this is a single setting and we don't have a desire to introduce many such settings.

- added migration that adds the `hide_title_and_feature_image` column to the `posts_meta` table with a `boolean` data type and a default value of `false` (matches behaviour of all existing pages)
- updated schema file for initial database creation
@kevinansfield kevinansfield force-pushed the add-hide-title-and-feature-image-property branch from 5740a12 to ceae2e2 Compare July 4, 2023 10:55
@kevinansfield
Copy link
Contributor Author

Full API implementation on top of this data change can be seen in 59d5223

@ronaldlangeveld ronaldlangeveld self-requested a review July 5, 2023 13:42
@kevinansfield kevinansfield merged commit 2a340bc into TryGhost:main Jul 5, 2023
20 checks passed
@kevinansfield kevinansfield deleted the add-hide-title-and-feature-image-property branch July 5, 2023 13:48
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.

2 participants