Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/repositories/member-repository.js (1)
359-403: Unnecessary transaction wrapper when free welcome email is inactive.When
source === 'member'butisFreeWelcomeEmailActiveisfalse,runMemberCreationis still wrapped inthis._Member.transaction()(line 398) even though there's nothing to do atomically — the outbox write is skipped. Pre-GA, the labs flag naturally bypassed this and fell through to the simplethis._Member.add(...)path. Now every member-source signup incurs a transaction wrapper and an extraAutomatedEmail.findOneread even on instances with no active welcome email.Consider short-circuiting to the simple path when the outbox write won't happen:
♻️ Optional refactor
- if (WELCOME_EMAIL_SOURCES.includes(source)) { - const freeWelcomeEmail = this._AutomatedEmail ? await this._AutomatedEmail.findOne({slug: MEMBER_WELCOME_EMAIL_SLUGS.free}) : null; - const isFreeWelcomeEmailActive = freeWelcomeEmail && freeWelcomeEmail.get('lexical') && freeWelcomeEmail.get('status') === 'active'; - const isFreeSignup = !stripeCustomer; - - const runMemberCreation = async (transacting) => { + const isFreeSignup = !stripeCustomer; + const freeWelcomeEmail = WELCOME_EMAIL_SOURCES.includes(source) && this._AutomatedEmail + ? await this._AutomatedEmail.findOne({slug: MEMBER_WELCOME_EMAIL_SLUGS.free}) + : null; + const isFreeWelcomeEmailActive = freeWelcomeEmail && freeWelcomeEmail.get('lexical') && freeWelcomeEmail.get('status') === 'active'; + + if (WELCOME_EMAIL_SOURCES.includes(source) && isFreeWelcomeEmailActive && isFreeSignup) { + const runMemberCreation = async (transacting) => {This way the
AutomatedEmailquery and transaction wrapper are only incurred when a welcome email will actually be sent. Theelsebranch (lines 404-410) handles all other cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/repositories/member-repository.js` around lines 359 - 403, The code always queries AutomatedEmail and wraps runMemberCreation in this._Member.transaction even when no outbox write will occur; to fix, short-circuit: move the AutomatedEmail.findOne and computation of isFreeWelcomeEmailActive behind a guard so you only compute them when WELCOME_EMAIL_SOURCES.includes(source) AND you may send the outbox (i.e., only when isFreeSignup can be true), and then only create/run runMemberCreation inside this._Member.transaction when isFreeWelcomeEmailActive && isFreeSignup is true; otherwise call this._Member.add(...) directly (avoid this._Member.transaction and avoid the AutomatedEmail.findOne read) and skip dispatching StartOutboxProcessingEvent. Use the existing symbols: WELCOME_EMAIL_SOURCES, isFreeSignup, isFreeWelcomeEmailActive, runMemberCreation, this._Member.transaction, this._Member.add, AutomatedEmail.findOne, and StartOutboxProcessingEvent.create to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/repositories/member-repository.js`:
- Around line 359-403: The code always queries AutomatedEmail and wraps
runMemberCreation in this._Member.transaction even when no outbox write will
occur; to fix, short-circuit: move the AutomatedEmail.findOne and computation of
isFreeWelcomeEmailActive behind a guard so you only compute them when
WELCOME_EMAIL_SOURCES.includes(source) AND you may send the outbox (i.e., only
when isFreeSignup can be true), and then only create/run runMemberCreation
inside this._Member.transaction when isFreeWelcomeEmailActive && isFreeSignup is
true; otherwise call this._Member.add(...) directly (avoid
this._Member.transaction and avoid the AutomatedEmail.findOne read) and skip
dispatching StartOutboxProcessingEvent. Use the existing symbols:
WELCOME_EMAIL_SOURCES, isFreeSignup, isFreeWelcomeEmailActive,
runMemberCreation, this._Member.transaction, this._Member.add,
AutomatedEmail.findOne, and StartOutboxProcessingEvent.create to locate and
change the logic.
|
The rabbit makes a good point here. Planning to address the comment before merging this one. |
cmraible
force-pushed
the
chris-ny-971-post-ga-cleanup-remove-the-welcomeemails-labs-flag-from-member-repo
branch
from
eaa59ec to
99c9fb7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/repositories/member-repository.js (1)
359-368:&& isFreeSignupin the branch condition is redundant
isFreeWelcomeEmailActivecan only ever betrueifshouldCheckFreeWelcomeEmailwastrue, and that already requiresisFreeSignup === true(line 360). The second&& isFreeSignupat line 368 is therefore dead-code as written today.♻️ Suggested simplification
- if (isFreeWelcomeEmailActive && isFreeSignup) { + if (isFreeWelcomeEmailActive) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/repositories/member-repository.js` around lines 359 - 368, The final if condition redundantly checks isFreeSignup because isFreeWelcomeEmailActive can only be set when shouldCheckFreeWelcomeEmail (which already requires isFreeSignup) is true; remove the extra "&& isFreeSignup" from the if that checks isFreeWelcomeEmailActive so it becomes simply "if (isFreeWelcomeEmailActive)". Keep the existing logic that sets isFreeWelcomeEmailActive via this._AutomatedEmail.findOne({slug: MEMBER_WELCOME_EMAIL_SLUGS.free}) and its status/lexical checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/repositories/member-repository.js`:
- Around line 359-368: The final if condition redundantly checks isFreeSignup
because isFreeWelcomeEmailActive can only be set when
shouldCheckFreeWelcomeEmail (which already requires isFreeSignup) is true;
remove the extra "&& isFreeSignup" from the if that checks
isFreeWelcomeEmailActive so it becomes simply "if (isFreeWelcomeEmailActive)".
Keep the existing logic that sets isFreeWelcomeEmailActive via
this._AutomatedEmail.findOne({slug: MEMBER_WELCOME_EMAIL_SLUGS.free}) and its
status/lexical checks.
Coderabbit's nit pick here is technically true, but I think I prefer to keep the |
| } | ||
|
|
||
| if (isFreeWelcomeEmailActive && isFreeSignup) { | ||
| const runMemberCreation = async (transacting) => { |
There was a problem hiding this comment.
This got a bit hairier than I expected for just removing a flag, but I'm pretty sure this is correct. Very open to suggestions/simplification
There was a problem hiding this comment.
yeah this file is always tricky. I think this is fine though, to me the logic is clear and easy to follow even if we could technically do it in fewer lines.
|
This should be the last one before removing the flag itself 😄 |
cmraible
force-pushed
the
chris-ny-971-post-ga-cleanup-remove-the-welcomeemails-labs-flag-from-member-repo
branch
from
2584233 to
44c2bf1
Compare
|
Sorry for the commits after requesting a review - I noticed there was some mocking still hanging around in the integration tests, then I rebased so I can stack the lab removal itself on top of this. |
| } | ||
|
|
||
| if (isFreeWelcomeEmailActive && isFreeSignup) { | ||
| const runMemberCreation = async (transacting) => { |
There was a problem hiding this comment.
yeah this file is always tricky. I think this is fine though, to me the logic is clear and easy to follow even if we could technically do it in fewer lines.
cmraible
deleted the
chris-ny-971-post-ga-cleanup-remove-the-welcomeemails-labs-flag-from-member-repo
branch
2 participants
ref https://linear.app/ghost/issue/NY-971/post-ga-cleanup-remove-the-welcomeemails-labs-flag-from-admin
closes https://linear.app/ghost/issue/NY-965/post-ga-cleanup-remove-labsservice-from-memberrepository
There should be no behavioral changes in this commit, since the
welcomeEmailsflag has already been promoted to GA.Now that welcome emails are in GA, we want to remove the flag completely. This commit removes any conditional behavior in Ghost/core's Member Repository that depends on the
welcomeEmailsflag.Note: this also removes the labsService dependency from the MembersRepository entirely.
I've manually tested the following: