Changed members-api constructor to accept Member model directly #105
Conversation
no issue - As members have become a part of Ghost core there is no need to proxy methods like this anymore and we can allow members-api to work on the model directly - Methods come from Ghost core: https://github.com/TryGhost/Ghost/blob/cc39786/core/server/services/members/api.js#L11-L110
There was a problem hiding this comment.
This is exactly the right direction of change 👍
Couple of points/ideas to think about, and these are just opinions, so take them as that
-
IMO the custom logic we have in
createMemberetc.. functions can be moved to the member model (e.g. default null for name/note) OR the members-api can be updated to deal with what the model expects/returns (though i still thinking callingtoJSONis a good idea, dealing with plain objects is nice IMO 🤷♂️) -
maybe the
getMetadatafn can be removed eventually and the member model can use some kind of relation plugin thing so that we can dowithRelated: stripeInfoStuff? -
maybe we eventually pass a model to the stripe module? maybe the member model, or maybe just the stripe related ones?
The idea behind passing in the functions before was so that members-api has NO idea what the datastore or model layer is, it just takes super basic functions that should never throw, just resolve with object or null - now that members is a core thing in Ghost, it's totally acceptable, and preferred that it used the same constructs as we do in Ghost.
The goal of this ticket is to remove as much "indirection" as possible with two main parts:
- Ghost core should be able to just use the model layer
- Members core should be able to just use the model layer
Obviously there's a long road to that, and this is a perfect start IMO
no issue - These methods have benn moved ton @tryghost/members-api in favor of using the model directly (ref: TryGhost/Members#105)
Moving this kind of logic into the model 💯 makes sense! As for
Wow. Just looked into this method and we definitely want to do it in a proper relational way 😬 The filter by member_id and reducing over the returned customer is def. not a nice way to do it 😂
Which module do you have in mind exactly? Thanks for the feedback! Will try moving the refactor in this direction bit by bit. Think it could become a nice blueprint in the end on how we could refactor out more of the modules out of Ghost core with their specialized functionalities in the future. |
😂 I fucking hate bookshelf relation plugin bullshit!! I spent a day on it and just went with this 😠
The
Yeah, same! I'm hoping it's small enough that it's not super confusing, but complex it enough that we can apply the same techniques to the rest of Ghost 💪 |
no issue - This includes the interface change for members-api constructor - now accepts the member's model instead of proxy methods. These methods have been moved ton @tryghost/members-api in favor of using the model directly (ref: TryGhost/Members#105) - Moved error handling from the service layer to controller - Bumped @tryghost/member-api package to 0.10.0
- This is the refactor similar to what has been done with Memeber model being passed in directly in the constructor - Relevent discussion here TryGhost#105 (review)
- This is the refactor similar to what has been done with Memeber model being passed in directly in the constructor - Relevent discussion here TryGhost#105 (review)
no issue - This is the refactor similar to what has been done with Memeber model being passed in directly in the constructor - Relevent discussion here #105 (review)
refs #105 - It's a follow up to a series of refactorings in the module mostly discussed in refed PR - The sendEmailWithMagicLink and destroyStripeSubscriptions were exposed through members API so that Ghost could call it from the controller level
refs TryGhost/Members#105 - Needs a version bump for @tryghost/members-api to use getStripeSubscriptions method - This new method is a little hacky because there are no relationships setup on members_* tables
refs TryGhost/Members#105 - As members module has become a core part it makes sense to follow the same principles as in all other controllers and use the model directly instead of calling external services. - Bumped @tryghost/members-api to 0.11.1 . New stripe-specific methods used in controllers are available starting with this version - Exposing these new methods is a little hacky because there are no relationships setup on members_* tables. Left notes for future improvements once relations are introduced. - We don't allow for chaging member's emails at the moment. For this reason had to modify JSON schema a little. It doesn't support OO inheritence: "This shortcoming is perhaps one of the biggest surprises of the combining operations in JSON schema: it does not behave like inheritance in an object-oriented language. " (ref. https://json-schema.org/understanding-json-schema/reference/combining.html#allof)
refs TryGhost/Members#105 - As members module has become a core part it makes sense to follow the same principles as in all other controllers and use the model directly instead of calling external services. - Bumped @tryghost/members-api to 0.11.1 . New stripe-specific methods used in controllers are available starting with this version - Exposing these new methods is a little hacky because there are no relationships setup on members_* tables. Left notes for future improvements once relations are introduced. - We don't allow for chaging member's emails at the moment. For this reason had to modify JSON schema a little. It doesn't support OO inheritence: "This shortcoming is perhaps one of the biggest surprises of the combining operations in JSON schema: it does not behave like inheritance in an object-oriented language. " (ref. https://json-schema.org/understanding-json-schema/reference/combining.html#allof)
refs TryGhost/Members#105 - As members module has become a core part it makes sense to follow the same principles as in all other controllers and use the model directly instead of calling external services. - Bumped @tryghost/members-api to 0.11.1 . New stripe-specific methods used in controllers are available starting with this version - Exposing these new methods is a little hacky because there are no relationships setup on members_* tables. Left notes for future improvements once relations are introduced. - We don't allow for chaging member's emails at the moment. For this reason had to modify JSON schema a little. It doesn't support OO inheritence: "This shortcoming is perhaps one of the biggest surprises of the combining operations in JSON schema: it does not behave like inheritance in an object-oriented language. " (ref. https://json-schema.org/understanding-json-schema/reference/combining.html#allof)
no issue - This is the refactor similar to what has been done with Memeber model being passed in directly in the constructor - Relevent discussion here TryGhost/Members#105 (review)
refs TryGhost/Members#105 - It's a follow up to a series of refactorings in the module mostly discussed in refed PR - The sendEmailWithMagicLink and destroyStripeSubscriptions were exposed through members API so that Ghost could call it from the controller level
Refactor that allows member-api to use Ghost member models directly, also reduces member-api constructor signature.
Note: would require a minor bump because of the constructor signature change
@allouis would be great if you could verify if I understood the needed refactor correctly and this is the right direction▶️