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

notif: Parse notification messages, on Android #333

Merged
merged 5 commits into from
Oct 25, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Oct 24, 2023

This PR is stacked atop #325, and makes the next step toward #320/#122.

Much of this is based on the Kotlin code that does this same job
in the zulip-mobile RN app:
  android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt
  android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt

Those Kotlin tests benefit from a feature of their test framework
where when an expectation like `expect.that(foo).isEqualTo(bar)`
(analogous to our `check(foo).equals(bar)`) is unsatisfied, it
records the failure without immediately ending the test function.
As a result, when something goes wrong we get very fine-grained
results, much like we would with package:checks if all the test
functions were translated as `group` and the individual expectations
as `test`, rather than as `check` -- but without the overhead that
the latter approach requires in the source code, particularly in
making up names for the individual test cases.

In this version, we go ahead and translate to `group` and `test` for
a couple of the test functions where their lack would be most keenly
felt, resorting to a hack to avoid cluttering the code with lots of
individual names.  We let the rest be just `test` and `check`.
The first version of this test file included these for ease of
comparison with zulip-mobile's FcmMessageTest.kt; but they
don't really add anything over the more detailed test cases
that follow.
In zulip-mobile we use this to identify the conversation in the case
of a 1:1 DM thread, as a legacy of senderId having not always been
present.  Here we'll use the numeric ID from the start.
@chrisbobbe chrisbobbe merged commit 7a3f13d into zulip:main Oct 25, 2023
@chrisbobbe
Copy link
Collaborator

LGTM, thanks! Merged.

@gnprice gnprice deleted the pr-notif-parse branch October 25, 2023 19:22
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.

2 participants