Skip to content
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

Group Preferences Actions #272

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Conversation

zombieobject
Copy link
Contributor

Introduction 📟

This Pull Request is intended to add Consent/Deny functionality to actions related to Group Messaging.

Discussion 🎙

The implementation follows closely with the work completed in the following Android SDK Pull Request

@zombieobject zombieobject self-assigned this Mar 1, 2024
@zombieobject zombieobject requested a review from a team as a code owner March 1, 2024 17:53
@zombieobject zombieobject enabled auto-merge (squash) March 1, 2024 17:53
Copy link
Contributor

@neekolas neekolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not enough of an xmtp-ios expert to approve, but love to see this coming to life

Comment on lines +128 to +135
public var senderHmac: Data {
get {return _senderHmac ?? Data()}
set {_senderHmac = newValue}
}
/// Returns true if `senderHmac` has been explicitly set.
public var hasSenderHmac: Bool {return self._senderHmac != nil}
/// Clears the value of `senderHmac`. Subsequent reads from it will return its default value.
public mutating func clearSenderHmac() {self._senderHmac = nil}
Copy link

@tuddman tuddman Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to this PR? I'm curious why this is needed here.

Copy link
Contributor Author

@zombieobject zombieobject Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was included in the required protobuf bump: 1eab635

Copy link

@tuddman tuddman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One ? about the inclusion of hmac functionality. Otherwise it looks very similar and good!

@zombieobject zombieobject merged commit ca76d64 into main Mar 1, 2024
2 checks passed
@zombieobject zombieobject deleted the em/group-preferences-actions branch March 1, 2024 19:12
@zombieobject
Copy link
Contributor Author

One ? about the inclusion of hmac functionality. Otherwise it looks very similar and good!

See my answer inline regarding the question. Thank you kindly for your time and attention to the review! 🫶🏼

@nplasterer
Copy link
Contributor

nplasterer commented Mar 2, 2024

@zombieobject Sorry for the late review on this. But looks like you missed updating one place
https://github.com/xmtp/xmtp-ios/blob/main/Sources/XMTPiOS/Conversation.swift#L33-L46

This whole method needs to be rewritten to better align with Android. https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/Conversation.kt#L91-L97

Could you do this in a follow up PR?

@zombieobject
Copy link
Contributor Author

zombieobject commented Mar 2, 2024

@zombieobject Sorry for the late review on this. But looks like you missed updating one place https://github.com/xmtp/xmtp-ios/blob/main/Sources/XMTPiOS/Conversation.swift#L33-L46

This whole method needs to be rewritten to better align with Android. https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/Conversation.kt#L91-L97

Could you do this in a follow up PR?

@nplasterer Thank you kindly for the time to review and the feedback. I'll be sure to follow up with a patch PR!

Update: Thanks for linking the diffs there. I can see clearly where the changes are needed. I think that I was only looking at the original PR and occasionally referencing the current source on Android at the time. Either way, I will improve my approach to diffing the changes between what was submitted in prior PR's vs what is currently in Android main at the time of my implementation.

This was great run at learning how the team works and I enjoy this attention to detail!

zombieobject added a commit that referenced this pull request Mar 4, 2024
When implementing change for Group Preferences Actions, the consentState function was not updated.

See PR: #272
@zombieobject
Copy link
Contributor Author

@nplasterer - Patch PR for these changes ready: #275

zombieobject added a commit that referenced this pull request Mar 5, 2024
When implementing change for Group Preferences Actions, the consentState function was not updated.

See PR: #272
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants