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

Add support for critical alerts #71

Merged
merged 7 commits into from
Aug 1, 2023

Conversation

Mstrodl
Copy link
Contributor

@Mstrodl Mstrodl commented May 24, 2023

Description

  • Adds critical alert support (See Apple's docs)
  • Allows for custom Serialize implementations for the APNS payload. This means users can add custom fields inside the aps key (a bad idea for new software, but unfortunately we're kinda stuck with old apps that read custom data from weird places)

How Has This Been Tested?

  • cargo test passes
  • Have been successfully sending pushes to our app with critical alerts and custom keys in aps.

Due Dilligence

Not sure if I'm filling these out correctly:

  • Breaking change (Users need to import PayloadLike. Maybe we could have dummy functions that replicate the functionality on the struct itself? Not sure if that'll work or if it's worth doing though)
  • Requires a documentation update (I believe I did this, so not sure if I should check it or not)
  • Requires a e2e/integration test update (Same as above)

Would really love for someone to code review this and let me know if there's anything I can improve on!

@Mstrodl
Copy link
Contributor Author

Mstrodl commented Jun 1, 2023

@HarryET Bumping this, since it seems like you're the maintainer

@HarryET
Copy link
Contributor

HarryET commented Jun 2, 2023

@HarryET Bumping this, since it seems like you're the maintainer

Hey @Mstrodl, yeah I'm maintaining it! I'm OOO for the rest of this week but will do a proper review next week, thanks for the PR 😄

@HarryET HarryET self-requested a review June 2, 2023 09:28
Copy link
Contributor

@HarryET HarryET left a comment

Choose a reason for hiding this comment

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

Hey @Mstrodl, this is a very well written PR! Thanks very much for the contribution I just had a couple questions to start which I added.

fn get_options(&self) -> &NotificationOptions;
}

impl<'a> PayloadLike for Payload<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use this now?

Copy link
Contributor Author

@Mstrodl Mstrodl Jun 5, 2023

Choose a reason for hiding this comment

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

So... my company's app puts some data into the aps key of the payload (bad idea, we know, we're kinda stuck with it though so older versions of the app can still receive pushes). Doing it this way means we can insert our own stuff into aps:

#[derive(Serialize, Debug, Clone)]
struct APSWrapper<'a> {
  r#type: &'a str,
  #[serde(flatten)]
  aps: APS<'a>,
}

#[derive(Serialize, Debug, Clone)]
struct Payload<'a> {
  aps: APSWrapper<'a>,
  details: IOSDetails<'a>,
  #[serde(serialize_with = "bson::serde_helpers::serialize_object_id_as_hex_string")]
  id: ObjectId,
  #[serde(skip_serializing)]
  device_token: &'a str,
  #[serde(skip_serializing)]
  options: NotificationOptions<'a>,
}

impl<'a> PayloadLike for Payload<'a> {
  fn get_device_token(&self) -> &'a str {
    self.device_token
  }

  fn get_options(&self) -> &NotificationOptions {
    &self.options
  }
}

The other bit that's nice about this is it lets us enforce types on all the keys (since we impl Serialize)

This could probably be a derive macro, but I don't really know how to do that yet 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 We send custom data too, using .add_custom_data, does that not work for this case too? If not it might be nice to extract this change into a separate PR so it can be reviewed separate to supporting critical notifications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add_custom_data won't let you add stuff inside the aps key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it would be nice to split it out into a separate PR, but the way I was writing my code (which was my smoke test) meant we needed to have this feature to make sure the whole chain worked

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'm going to ping someone else internally to review as well then as this is a breaking change

src/request/notification/default.rs Outdated Show resolved Hide resolved
@Mstrodl
Copy link
Contributor Author

Mstrodl commented Jun 5, 2023

@HarryET Made some changes... Let me know what you think!

@HarryET
Copy link
Contributor

HarryET commented Jun 6, 2023

@HarryET Made some changes... Let me know what you think!

Very nice, makes .set_critical() easier to read, do we have tests for that custom serde seralizer/deseralizer?

@Mstrodl
Copy link
Contributor Author

Mstrodl commented Jun 9, 2023

@HarryET Made some changes... Let me know what you think!

Very nice, makes .set_critical() easier to read, do we have tests for that custom serde seralizer/deseralizer?

Yes, because everything goes through PayloadLike, there shouldn't be any difference. I also have an example of the full thing in the docs, but it's not run automatically because it would need credentials

@HarryET HarryET requested review from xav and Rakowskiii June 14, 2023 09:33
@HarryET
Copy link
Contributor

HarryET commented Jun 14, 2023

@HarryET Made some changes... Let me know what you think!

Very nice, makes .set_critical() easier to read, do we have tests for that custom serde seralizer/deseralizer?

Yes, because everything goes through PayloadLike, there shouldn't be any difference. I also have an example of the full thing in the docs, but it's not run automatically because it would need credentials

Okay, a small test case might be nice but if it's already included by proxy that should be ok

@Mstrodl
Copy link
Contributor Author

Mstrodl commented Jun 23, 2023

Any news on those reviews?

Copy link
Contributor

@HarryET HarryET left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, looks good will be a major version bump due to the breaking PayloadLike change

@HarryET HarryET merged commit 7242a9a into WalletConnect:master Aug 1, 2023
5 checks passed
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.

None yet

2 participants