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

fix(message overrides): extract message content with Platform-Specific Overrides #12917

Conversation

buddyeorl
Copy link
Contributor

Description of changes

  • Exposed getClientInfo function from @aws-amplify/core
  • Refactored the extractContent function to accept a platform argument and use it to extract the correct platform override button actions and links from the message.

Issue #, if available

Issue 12915

Description of how you validated changes

  • Set up pinpoint in app messages with android, and ios and web overrides
  • Used Verdaccio to install updated packages on test apps, react native android, ios and react app.
  • Confirmed that overrides where extracted correctly and the button actions triggered the correct event.
  • Repeated the steps setting up pinpoint in app messages, with no overrides and with only single platform overrides.

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@buddyeorl buddyeorl requested review from a team as code owners January 28, 2024 18:03
@buddyeorl buddyeorl changed the title extract message content with Platform-Specific Overrides fix(message overrides): extract message content with Platform-Specific Overrides Jan 28, 2024
Copy link
Member

@Samaritan1011001 Samaritan1011001 left a comment

Choose a reason for hiding this comment

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

@buddyeorl, appreciate this very much. I added a few questions and suggestions. Looking forward to work with you to get this in.

Copy link
Member

@Samaritan1011001 Samaritan1011001 left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. Just one more important core export update needed but otherwise it looks good to me!

packages/notifications/__tests__/testUtils/data.ts Outdated Show resolved Hide resolved
packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/types/core.ts Outdated Show resolved Hide resolved
@buddyeorl
Copy link
Contributor Author

Overall looks pretty close to me. I just have a few suggestions to improve maintainability. I also think there are some formatting issues - I know our repo is currently not set up to auto-format but it would be helpful if you could run these changes through Prettier (the repo should have prettier settings configured)

@cshfang, Oops, I'm on a new machine and realized I hadn't set up any plugins in my IDE. I've now added Prettier and reformatted all the modified/added code. It should be good now:
prettier

Copy link
Member

@cshfang cshfang left a comment

Choose a reason for hiding this comment

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

One last small change and I think we're probably good to go. Thanks for bearing with me on this

configPlatform: ButtonConfigPlatform,
button?: InAppMessageButton,
): InAppMessageButton['DefaultConfig'] | undefined => {
if (!button || !button?.DefaultConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think this is effectively the same as

if (!button?.DefaultConfig) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 373 to 379
if (!configPlatform || !button?.[configPlatform]) {
return button?.DefaultConfig;
}
return {
...button?.DefaultConfig,
...button?.[configPlatform],
};
Copy link
Member

Choose a reason for hiding this comment

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

Given the guard clause above and how the spread operator works, I think we can actually condense this:

return {
  button.DefaultConfig,
  button.configPlatform,
}

Since we know button is guaranteed to be truthy by this point and even if configPlatform is nullish, it would spread just fine and we would end up returning the DefaultConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cshfang
Copy link
Member

cshfang commented Feb 19, 2024

Overall looks pretty close to me. I just have a few suggestions to improve maintainability. I also think there are some formatting issues - I know our repo is currently not set up to auto-format but it would be helpful if you could run these changes through Prettier (the repo should have prettier settings configured)

@cshfang, Oops, I'm on a new machine and realized I hadn't set up any plugins in my IDE. I've now added Prettier and reformatted all the modified/added code. It should be good now: prettier

No worries - really appreciate it

cshfang
cshfang previously approved these changes Mar 5, 2024
@Samaritan1011001
Copy link
Member

@buddyeorl Apologies for the delay here. I was finally able to verify the changes on both Android and iOS devices work properly. However, there are a few lint fixes you'll have to make before we merge. The failing test should give you the info needed. Looking to merge once these are updated.

@buddyeorl buddyeorl dismissed stale reviews from Samaritan1011001 and cshfang via aa52fe7 April 16, 2024 04:36
@buddyeorl
Copy link
Contributor Author

@buddyeorl Apologies for the delay here. I was finally able to verify the changes on both Android and iOS devices work properly. However, there are a few lint fixes you'll have to make before we merge. The failing test should give you the info needed. Looking to merge once these are updated.

@Samaritan1011001 Thanks for the update! I've addressed the lint fixes and re-run the tests on my end. Everything should be passing smoothly now. Please let me know if there's anything else needed from my side. Appreciate your assistance!

@Samaritan1011001 Samaritan1011001 merged commit cb91437 into aws-amplify:main Apr 18, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants