-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(community): send signals about member reevaluation in progress #5120
feat(community): send signals about member reevaluation in progress #5120
Conversation
Jenkins BuildsClick to see older builds (4)
|
protocol/communities/manager.go
Outdated
// Publish when the reevluation started since it can take a while | ||
signal.SendCommunityMemberReevaluationStarted(community.IDString()) | ||
|
||
newPrivilegedMembers, err := m.ReevaluateMembers(community) | ||
|
||
// Publish the reevaluation ending, even if it errored | ||
// A possible improvement would be to pass the error here | ||
signal.SendCommunityMemberReevaluationEnded(community.IDString()) | ||
|
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.
I thin it would be better to keep it inside ReevaluateMembers
maybe? 🤔
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.
It's simpler to put it in reevaluateCommunityMembersPermissions
, because it's the one that gets called by others and the only one that calls ReevaluateMembers
.
Also, I want to call SendCommunityMemberReevaluationEnded
whether it is a failure or not, and doing that in ReevaluateMembers
would be a pain, because I'd have to put it before every return
s
const ( | ||
MemberReevaluationStarted = "community.memberReevaluationStarted" | ||
MemberReevaluationEnded = "community.memberReevaluationEnded" | ||
) | ||
|
||
type CommunityMemberReevaluationSignal struct { | ||
CommunityID string `json:"communityId"` | ||
} |
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.
what if instead MemberReevaluationStarted
and MemberReevaluationEnded
we will register MemberReevaluationStatus
and return CommunityMemberReevaluationSignal
with the status (enum: none, started/inProgress, finished)? In this case, on the NIM side, you can decrease the amount of the code too, because you'll not need to do separate methods for handling started/ended
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.
Updated
9623775
to
16655f7
Compare
16655f7
to
f2ad9ac
Compare
@@ -1227,7 +1227,15 @@ func (m *Manager) DeleteCommunityTokenPermission(request *requests.DeleteCommuni | |||
} | |||
|
|||
func (m *Manager) reevaluateCommunityMembersPermissions(communityID types.HexBytes) error { | |||
// Publish when the reevluation started since it can take a while | |||
signal.SendCommunityMemberReevaluationStarted(types.EncodeHex(communityID)) |
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.
It would be great if we could use MessengerSignalsHandler
instead. Globals are bad 👮
Example:
if m.config.messengerSignalsHandler != nil {
m.config.messengerSignalsHandler.MessengerResponse(response)
}
Needed for status-im/status-desktop#14378
Sends signals when the member re-evaluation is ongoing