Conversation
toger5
changed the title
toger5
force-pushed
the
toger5/membership-manager-one-send-delayed-action
branch
from
005398a to
041c915
Compare
toger5
changed the title
hughns
reviewed
Mar 24, 2025
Member
hughns
left a comment
hughns
left a comment
There was a problem hiding this comment.
I've had an initial pass over this, but I think I will need to look again with fresh eyes.
Whilst the PR could be split out into a refactor and the logic change I found it easy enough to split out whilst reviewing.
hughns
changed the title
4 tasks
toger5
force-pushed
the
toger5/membership-manager-one-send-delayed-action
branch
from
e193efc to
c77db51
Compare
BillCarsonFr
approved these changes
Mar 25, 2025
Member
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
Just read the code to get a better understanding of membership manager. I have question
We do already have the state `hasMemberEvent` that allows to distinguish the two cases. No need to create two dedicated actions.
- deprecate isJoined (replaced by isActivated) - move Interface types to types.ts
toger5
force-pushed
the
toger5/membership-manager-one-send-delayed-action
branch
from
5acd034 to
052cef9
Compare
hughns
approved these changes
Mar 25, 2025
Member
hughns
left a comment
hughns
left a comment
There was a problem hiding this comment.
With fresh eyes and the updated comments this looks good 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We do already have the state
hasMemberEventthat allows to distinguish the two cases. No need to create two dedicated actions.This also moves code from
NewMemberhsipManager.ts->types.ts.And we expose status through an event emitter for the membership manager.
Checklist
public/exportedsymbols have accurate TSDoc documentation.