-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Cleaned up members isPaid
flag in settings table
#11651
Conversation
refs TryGhost/Ghost#11651 - The flag is being removed from the backend, so can be safely removed on client as well
core/server/data/migrations/versions/3.11/02-remove-legacy-is-paid-flag-from-settings.js
Outdated
Show resolved
Hide resolved
refs TryGhost#11650 - The flag is no longer used and can be removed to make the settings JSON easier to read
8e51242
to
9efdd55
Compare
paymentProcessor => paymentProcessor.adapter === 'stripe' | ||
); | ||
|
||
if (stripePaymentProcessor && stripePaymentProcessor.config.public_token && stripePaymentProcessor.config.secret_token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allouis does this logic make sense to you? Couldn't find any concrete reference to equivalent of this flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might wanna doublecheck with @rishabhgrg who introduced the flag, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gargol Think this is fine, isPaid
was to mark if stripe setup is present or not so this should ideally cover it. 🤔
core/server/data/migrations/versions/3.12/01-remove-legacy-is-paid-flag-from-settings.js
Show resolved
Hide resolved
core/server/data/migrations/versions/3.12/01-remove-legacy-is-paid-flag-from-settings.js
Outdated
Show resolved
Hide resolved
const settingsKey = 'members_subscription_settings'; | ||
|
||
return localOptions | ||
.transacting('settings') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this not be more specific like:
.transacting('settings')
.where('key', settingsKey)
.select('value')
.first()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! I like it. Will redo the query 👍
refs TryGhost/Ghost#11651 - The flag is being removed from the backend, so can be safely removed on client as well
While working on #11650 spotted this flag and decided to do a lil cleanup while in the area.
The
isPaid
flag is no longer used and can be removed to make the settings JSON easier to readTODO: