🐛 Prevent premature deletion of complimentary subscriptions on SQLite installs. #22178
Conversation
WalkthroughThe code update introduces a new dependency, ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/members/jobs/clean-expired-comped.js (2)
35-44: Core fix looks good! Consider adding error handling.The implementation correctly handles the date format differences between MySQL and SQLite, fixing the premature deletion issue. However, consider these improvements:
- Add error handling for database detection
- Add comments explaining the date format differences
let expiredCompedRows = []; +// MySQL stores dates in ISOString format while SQLite uses epoch milliseconds +// This conditional ensures correct date comparison for each database type if (DatabaseInfo.isMySQL(db.knex)) { + try { expiredCompedRows = await db.knex('members_products') .where('expiry_at', '<', moment.utc().startOf('day').toISOString()) .select('*'); + } catch (error) { + debug('Failed to query expired comped subscriptions: ', error); + throw error; + } } else { + try { expiredCompedRows = await db.knex('members_products') .where('expiry_at', '<', moment.utc().startOf('day').valueOf()) .select('*'); + } catch (error) { + debug('Failed to query expired comped subscriptions: ', error); + throw error; + } }
52-97: Add error handling for cleanup operations.While the cleanup logic is solid, consider adding error handling for the critical operations to ensure graceful failure and proper logging.
if (expiredCompedRows?.length) { + try { const rowIds = expiredCompedRows.map(d => d.id); const memberIds = expiredCompedRows.map(d => d.member_id); // Delete all expired comped rows deletedExpiredSubs = await db.knex('members_products') .whereIn('id', rowIds) .del(); // fetch all comped members to update const membersToUpdate = await db.knex('members') .whereIn('id', memberIds) .andWhere('status', 'comped'); const updateMemberIds = membersToUpdate.map(d => d.id); // Update all comped members to free updatedMembers = await db.knex('members') .whereIn('id', updateMemberIds) .update({ status: 'free' }); const statusEvents = membersToUpdate.map((member) => { const now = db.knex.raw('CURRENT_TIMESTAMP'); return { id: ObjectId().toHexString(), member_id: member.id, from_status: member.status, to_status: 'free', created_at: now }; }); // SQLite >= 3.32.0 can support 32766 host parameters // each row uses 5 variables so ⌊32766/5⌋ = 6553 const chunkSize = 6553; const chunks = chunkArray(statusEvents, chunkSize); // Adds status event for members going comped->free for (const chunk of chunks) { await db.knex('members_status_events').insert(chunk); } + } catch (error) { + debug('Failed during cleanup operations: ', error); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/members/jobs/clean-expired-comped.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
🔇 Additional comments (1)
ghost/core/core/server/services/members/jobs/clean-expired-comped.js (1)
6-6: LGTM! Appropriate dependency added for database type detection.The addition of
DatabaseInfois a good solution for handling database-specific date format differences.
| if (DatabaseInfo.isMySQL(db.knex)) { | ||
| expiredCompedRows = await db.knex('members_products') | ||
| .where('expiry_at', '<', moment.utc().startOf('day').toISOString()) | ||
| .select('*'); | ||
| } else { | ||
| expiredCompedRows = await db.knex('members_products') | ||
| .where('expiry_at', '<', moment.utc().startOf('day').valueOf()) | ||
| .select('*'); | ||
| } |
There was a problem hiding this comment.
@cathysarisky I think it could be useful to leave a comment here explaining why we need to do this
There was a problem hiding this comment.
We could also maybe reduce a little duplication here:
const expiryDate = DatabaseInfo.isMySQL(db.knex)
? moment.utc().startOf('day').toISOString()
: moment.utc().startOf('day').valueOf();
const expiredCompedRows = await db.knex('members_products')
.where('expiry_at', '<', expiryDate)
.select('*');
There was a problem hiding this comment.
Would you mind adding a test that verifies this change does what is intended? It looks like there isn't much coverage around this area, but this may be a good reference to base the test on
|
An alternative: fix the root problem, which is that this field is stored differently than other sqlite dates: Ghost/ghost/core/core/server/models/member.js Line 226 in e60bbae If that were const expiry = product.expiry_at ? new Date(product.expiry_at).toISOString().slice(0, 19).replace('T', ' ') : null; (or the equivalent with moment?), what goes into SQLite would match what's already happening in MySQL, and the bug would disappear. Thoughts? |
closes #22176
Development installs with SQLite store dates as epoch milliseconds. MySQL stores ISOStrings. The clean-expired-comps job, which is supposed to detect complimentary subscriptions that have expired, was erroneously comparing an ISOString to the SQLite field, which was epoch milliseconds. The result was premature removal of comp subscription that had not yet expired, but only on SQLite sites.
Kudos to Ravn on the Ghost forum for helping to identify the behavior! (Ref: https://forum.ghost.org/t/bulk-creation-of-comped-users-via-admin-api-expiration-issue-after-approx-15-hours/55244/7 )