Skip to content

Require Android users on pre-16.2.96 releases to upgrade. #10957

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

Closed
wants to merge 8 commits into from

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Dec 5, 2018

This will enable us to roll out "remove notification when read" (zulip/zulip-mobile#3119) to all Android users.

This will facilitate adding a bunch more test cases shortly.
This release is from 2018-08-22, a little over 100 days ago.

It was the first release with the important fix so that when the
server advises it to stop displaying a notification because the user
has read the message (as the SEND_REMOVE_PUSH_NOTIFICATIONS server
setting enables), the app doesn't instead replace the notification
with a broken one reading "null".  We have that setting running now
on chat.zulip.org, and intend to roll it out more broadly soon.

The `# take 0` thing is a slightly absurd workaround for the fact
that our funky out-of-line way of marking lines to ignore doesn't
work right if there are multiple such lines in a given file that
are equal modulo leading and trailing whitespace.
In a quick scan of today's nginx logs on chat.zulip.org, there
were 20 distinct user-agents that begin with 'ZulipMobile/'.
Here's a representative sampling of them, such that the rest
were all boringly similar to one of these.

First, to make room for these without an excess of copy-paste and
overlong lines, convert this test to a data-oriented style.  The
existing, synthetic cases appear in the new data followed by the
seen-in-the-wild cases.

Happily, the code being tested passes all these new cases unchanged.
@timabbott
Copy link
Member

@gnprice this looks great! The one thing I'd like us to adjust here is to send a code for this JSON error, so that in future versions, we don't need to keep that hardcoded string.

I merged this, with a better linter strategy, as 9de1bd4.

Thanks @gnprice!

@timabbott timabbott closed this Dec 5, 2018
@gnprice gnprice deleted the compatibility branch December 6, 2018 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants