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

MOB-10364 Support for In App JSON Only Messages #857

Merged
merged 32 commits into from
Jan 10, 2025

Conversation

sumeruchat
Copy link
Contributor

@sumeruchat sumeruchat commented Jan 3, 2025

🔹 Jira Ticket(s) if any

✏️ Description

Support for JSON Only in app messages

@sumeruchat sumeruchat marked this pull request as draft January 3, 2025 13:17
@sumeruchat sumeruchat changed the base branch from master to feature/MOB-10521-prepare-3.5.6-release January 3, 2025 13:17
@sumeruchat sumeruchat closed this Jan 3, 2025
@sumeruchat sumeruchat reopened this Jan 3, 2025
Base automatically changed from feature/MOB-10521-prepare-3.5.6-release to master January 3, 2025 17:16
@sumeruchat sumeruchat changed the title MOB-10364 Support for In App JSON Only Messages (WIP) MOB-10364 Support for In App JSON Only Messages Jan 7, 2025
@sumeruchat sumeruchat marked this pull request as ready for review January 7, 2025 16:12
@@ -414,7 +419,8 @@ private void processMessages() {
InAppResponse response = handler.onNewInApp(message);
IterableLogger.d(TAG, "Response: " + response);
message.setProcessed(true);
if (response == InAppResponse.SHOW) {

if (response == InAppResponse.SHOW || message.isJsonOnly()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@sumeruchat I might be mis-reading this, but should this be !message.isJsonOnly() here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it looks like showMessage immediately discards it. Sorry for the noise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, circling back — why pass it to showMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line means we "show" the message and ignore whatever the return value from onNew was. In the showMessage method we will just simply consume jsonOnly messages without actually showing them. I agree it looks like a mistake should I try to do it differently?

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'll think about it and change this just for clarity later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. This isn't a big deal — feel free to leave it if you think that's best! Maybe just a comment?

setRead(message, true, null, null);
message.setConsumed(true);
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradumbaugh here we will make sure we dont actually "show" jsonOnly messages

Copy link
Contributor

@evantk91 evantk91 left a comment

Choose a reason for hiding this comment

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

Left a few comments. We will look at the end-to-end test tomorrow. It seems that I am getting a jsonOnly message with flag set to false when I send a json only message from the backend.


message.inAppStorageInterface = storageInterface;
if (html != null) {
if (content.html != null && content.html.length() > 0 && !jsonOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was suggested to be replaced with !content.html.isEmpty()

Copy link
Contributor

@evantk91 evantk91 left a comment

Choose a reason for hiding this comment

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

Approved. Updated the parsing of JSON only flag and tested end-to-end.

@sumeruchat sumeruchat merged commit bd6cf8d into master Jan 10, 2025
2 of 3 checks passed
@sumeruchat sumeruchat deleted the feature/MOB-10364-in-app-json-only branch January 10, 2025 23:12
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.

3 participants