Feat: Respect Server Typing Duration Settings & Tests #1448

criticic · 2023-12-09T17:08:47Z

What does this PR do, and why?

As introduced in ZFL 204/Zulip 8.0, the typing duration values should no longer be hardcoded into the clients, and the server will provide those values. In case the values are not present, the previous defaults are used. I have also added tests to check for this behaviour.

Outstanding aspect(s)

[ ]

External discussion & connections

Discussed in #zulip-terminal in Respect server typing notification settings #T1448 #T1445
Fully fixes Respect server typing notification duration settings #1445
Partially fixes issue #
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

criticic · 2023-12-09T17:54:07Z

@neiljp Kindly review. I am unsure about the commit message style, though I have rebased the commits into one.
Let me know if I should make any changes.

criticic · 2023-12-09T18:03:55Z

@zulipbot add "PR needs review"

rsashank

@criticic, appreciate your efforts on this PR!

Consider rebasing and squashing both commits into a single one. Creating an extra commit solely for addressing linting issues might be causing the isolated commits test failure (where each commit needs to individually pass tests).

Additionally, I'd like to mention a previous suggestion I received: consider setting up gitlint to review your commit messages style and take a look at the repo's commit history for further reference.

rsashank

Can you attempt to rerun the check? I'm not too sure as to why it's failing, given that it succeeded in the PyPy 3.8 & 3.9 check.

---
This is a known issue apparently, it has nothing to do with the PR.

neiljp

@criticic Thanks for the PR - and for your patience in waiting for feedback 👍

The code in this PR looks fine to fix the issue 👍 To improve the quality of this PR and help with future contributions, I'd suggest considering the commit structure itself, as below.

My primary feedback would be that multiple things are changing in the one commit:

the units are moving from seconds to milliseconds
the UI code is moving to use the new model API (attributes)
the model API values become dependent upon the server values

The latter is the key feature change for #1445, whereas the others are refactors which enable it. Separating these makes it easier to pin down which commits should change behavior and which should not, as well as use the commit text to describe each specific change. This helps with reviewing, and can highlight a lack of tests and other aspects. Commits can always be squashed together right before merging if it seems like a good idea, but splitting up can be more challenging as a final step :)

Note that the failed linting from earlier shouldn't occur when you keep each commit up to date individually.

Splitting this up into smaller commits would also give more experience following the commit style we use, as noted in the README, and which will be helped by using gitlint.

Please check in on chat.zulip.org if you're unsure about anything :)

neiljp · 2024-01-09T02:20:06Z

@criticic As per @rsashank's point, rebasing against the latest main branch, after I merged the temporary fix for CI, should fix the failure for pypy37.

criticic · 2024-01-09T05:11:34Z

@neiljp thanks for the detailed feedback, ill implement the changes soon

criticic · 2024-02-02T20:51:37Z

@zulipbot add "PR needs review"

neiljp

@criticic Thanks for the update - can you see how the commit structure here communicates the changes cleanly?

I've left some further feedback, but it's mainly guidelines on test styling.

As per my first review, the core code remains fine 👍 I believe this was your first PR, so I hope you appreciate and understand how the changes in tests and commit structure help overall - and that these changes haven't slowed you down too much.

neiljp · 2024-02-02T23:54:40Z

tests/model/test_model.py

+    @pytest.mark.parametrize(
+        "feature_level",
+        [None, 185, 203],
+        ids=[
+            "Zulip_2.1.x_ZFL_None_no_settings",
+            "Zulip_7.0.x_ZFL_185_no_settings",
+            "Zulip_8.0.x_ZFL_203_no_settings",
+        ],
+    )
+    def test__store_typing_duration_settings_default_values(


This is broadly a great initial test, and I thought initially you might have planned to extend this to other values in the next commit, which would make it a little less strange to see this parametrize. However, you did split out the other test part, and in any case, to someone just reading this commit, it is a little strange to have these values in the parametrize given the behavior is identical for each value/id.

Typically I'd expect to see a test with no parametrize when a test is first introduced like this.

Including this split would be great when things are dependent upon feature level, so the parametrize part could be appropriate to the next commit.

neiljp · 2024-02-03T00:08:02Z

tests/model/test_model.py

+    @pytest.mark.parametrize(
+        "feature_level, to_vary_in_initial_data",
+        [
+            (
+                204,
+                {
+                    "server_typing_started_wait_period_milliseconds": 7500,
+                    "server_typing_stopped_wait_period_milliseconds": 3000,
+                    "server_typing_started_expiry_period_milliseconds": 10000,
+                },
+            ),
+        ],
+        ids=["Zulip_8.0.x_ZFL_204_with_settings"],
+    )
+    def test__store_typing_duration_settings_with_values(
+        self, model, initial_data, feature_level, to_vary_in_initial_data
+    ):


Ah, I see you split out this new test.

Since you have only one entry in your parametrize, it isn't necessary to use parametrize (though some old tests we have still do for now!). You can still set values like feature_level and to_vary_in_initial_state, and we often do so in the parameter list to the test (rather than simply in the test body), since that allows us to switch back to using parametrize later fairly easily, if it becomes necessary.

You may find it simpler to extract the 3 dict parameters out manually here, since it will make the asserts easier to read :)

Lastly, it would be useful to add asserts in the setup here, that the input parameters are not simply the same as the defaults - that would avoid this test accidentally passing.

tests/model/test_model.py

criticic · 2024-02-03T05:36:01Z

@zulipbot add "PR needs review"

From ZFL 204, the server provides typing notification parameters in milliseconds; this change ensures that the default values are also in milliseconds (instead of seconds).

Stores the default typing notification parameters in the model. Code which previously accessed these values directly now queries the model instead. A test has been added to verify the new model values.

From ZFL 204, the typing notification durations were made server-configurable; this change uses those values when provided, or else uses the default values. A test has been added to check this behaviour. Fixes zulip#1445.

neiljp · 2024-02-05T23:12:07Z

@criticic Thanks for the last update!

I made some minor changes to the commit text and pushed back here, along with some small moves in the test you added in the last commit, to illustrate the points I made above.

I'll merge this after it passes CI, which shouldn't be a problem :)

zulipbot added the size: L [Automatic label added by zulipbot] label Dec 9, 2023

criticic marked this pull request as draft December 9, 2023 17:10

criticic force-pushed the feat/respect-server-typing-settings branch from 82a5be9 to 80054aa Compare December 9, 2023 17:30

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Dec 9, 2023

criticic marked this pull request as ready for review December 9, 2023 17:48

zulipbot added the PR needs review PR requires feedback to proceed label Dec 9, 2023

criticic force-pushed the feat/respect-server-typing-settings branch 2 times, most recently from a0684f7 to 5460544 Compare December 10, 2023 07:09

rsashank reviewed Jan 1, 2024

View reviewed changes

criticic force-pushed the feat/respect-server-typing-settings branch from 5460544 to e14d33a Compare January 4, 2024 08:49

criticic requested a review from rsashank January 4, 2024 08:50

rsashank reviewed Jan 7, 2024

View reviewed changes

neiljp reviewed Jan 9, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jan 9, 2024

criticic force-pushed the feat/respect-server-typing-settings branch 2 times, most recently from 7242309 to f06c9d7 Compare January 9, 2024 06:58

zulipbot added size: S [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Jan 9, 2024

criticic force-pushed the feat/respect-server-typing-settings branch from f06c9d7 to 4ebd70d Compare January 9, 2024 07:01

zulipbot added size: L [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Jan 9, 2024

criticic force-pushed the feat/respect-server-typing-settings branch from 87e43ab to e869900 Compare January 9, 2024 07:15

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jan 9, 2024

neiljp removed the PR needs review PR requires feedback to proceed label Jan 25, 2024

criticic force-pushed the feat/respect-server-typing-settings branch 5 times, most recently from 1f78ab9 to ddd2fdf Compare February 2, 2024 20:42

criticic requested a review from neiljp February 2, 2024 20:51

zulipbot added the PR needs review PR requires feedback to proceed label Feb 2, 2024

neiljp reviewed Feb 3, 2024

View reviewed changes

neiljp added api migrations version parity: 8 and removed PR needs review PR requires feedback to proceed labels Feb 3, 2024

criticic force-pushed the feat/respect-server-typing-settings branch from ddd2fdf to 89440c1 Compare February 3, 2024 05:26

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Feb 3, 2024

zulipbot added the PR needs review PR requires feedback to proceed label Feb 3, 2024

criticic requested a review from neiljp February 3, 2024 05:36

criticic added 3 commits February 5, 2024 13:25

refactor: boxes/api_types: Use milliseconds for typing notifications.

17bb308

From ZFL 204, the server provides typing notification parameters in milliseconds; this change ensures that the default values are also in milliseconds (instead of seconds).

refactor: boxes/model: Store typing notification durations in model.

1051608

Stores the default typing notification parameters in the model. Code which previously accessed these values directly now queries the model instead. A test has been added to verify the new model values.

neiljp force-pushed the feat/respect-server-typing-settings branch from 89440c1 to 6e8c47e Compare February 5, 2024 23:12

neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Feb 5, 2024

neiljp added this to the Next Release milestone Feb 5, 2024

neiljp merged commit eeffc8c into zulip:main Feb 5, 2024
20 checks passed

neiljp mentioned this pull request Feb 22, 2024

Support server-customized presence parameters. #1463

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Respect Server Typing Duration Settings & Tests #1448

Feat: Respect Server Typing Duration Settings & Tests #1448

criticic commented Dec 9, 2023 •

edited by neiljp

Loading

criticic commented Dec 9, 2023 •

edited

Loading

criticic commented Dec 9, 2023

rsashank left a comment

rsashank left a comment •

edited

Loading

neiljp left a comment

neiljp commented Jan 9, 2024

criticic commented Jan 9, 2024

criticic commented Feb 2, 2024

neiljp left a comment

neiljp Feb 2, 2024

neiljp Feb 3, 2024

criticic commented Feb 3, 2024

neiljp commented Feb 5, 2024

Feat: Respect Server Typing Duration Settings & Tests #1448

Feat: Respect Server Typing Duration Settings & Tests #1448

Conversation

criticic commented Dec 9, 2023 • edited by neiljp Loading

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Self-review checklist for each commit

criticic commented Dec 9, 2023 • edited Loading

criticic commented Dec 9, 2023

rsashank left a comment

Choose a reason for hiding this comment

rsashank left a comment • edited Loading

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

neiljp commented Jan 9, 2024

criticic commented Jan 9, 2024

criticic commented Feb 2, 2024

neiljp left a comment

Choose a reason for hiding this comment

neiljp Feb 2, 2024

Choose a reason for hiding this comment

neiljp Feb 3, 2024

Choose a reason for hiding this comment

criticic commented Feb 3, 2024

neiljp commented Feb 5, 2024

criticic commented Dec 9, 2023 •

edited by neiljp

Loading

criticic commented Dec 9, 2023 •

edited

Loading

rsashank left a comment •

edited

Loading