-
Notifications
You must be signed in to change notification settings - Fork 217
Implement MSC3912: redaction with relations #1688
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
Conversation
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.
LGTM 😎
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.
Some cosmetic changes but LGTM :)
MatrixSDK/Data/MXRoom.m
Outdated
return [mxSession.matrixRestClient redactEvent:eventId withRelations:relations inRoom:self.roomId reason:reason txnId:nil featureIsStable:isFeatureStable success:success failure:failure]; | ||
} else { | ||
MXLogDebug(@"[MXRoom] redaction with relations is not supported (MSC3912)"); | ||
return [self redactEvent:eventId reason:reason success:success failure:failure]; |
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.
Shouldn't we mark this method as deprecated since it is called by - (MXHTTPOperation*)redactEvent:(NSString*)eventId withRelations:(NSArray<NSString *>*)relations reason:(NSString*)reason success:(void (^)(void))success failure:(void (^)(NSError *error))failure
if the server doesn't support this new MSC?
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.
Thx @gileluard , I checked this point with @nimau. Finally he will call here the new implemented redact function (based on the actual redact endpoint) by ignoring the relations
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.
@gileluard: I fixed it in the last commit. Now the fallback (if the MSC3912 is not supported) will be on the new endpoint but without passing any relation types.
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.
LGTM, only one comment
MatrixSDK/Data/MXRoom.h
Outdated
withRelations:(NSArray<NSString *>*)relations | ||
reason:(NSString*)reason | ||
success:(void (^)(void))success | ||
failure:(void (^)(NSError *error))failure NS_REFINED_FOR_SWIFT; |
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.
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.
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.
Oh my bad, I missed that. Implementing it in Swift would probably be best to my mind. Might be possible to add the extra parameter to the existing method with a default value.
Co-authored-by: Gil Eluard <[email protected]>
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.
SGTM
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.
lgtm 👍
SDK support for MSC3912