Simplify Zulip Feature Level Handling. #1462

rsashank · 2024-01-17T17:04:46Z

What does this PR do, and why?

Fixes #1364
Pure integer representation replacing the current None value when ZFL is absent.

Outstanding aspect(s)

[ ]

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes Simplify Zulip Feature Level (ZFL) handling (treat absent as zero) #1364
Partially fixes issue #
Builds upon previous unmerged work in PR Refactor: Zulip Feature Level simplified #1405
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

Visual changes

zulipbot · 2024-01-17T17:04:55Z

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

neiljp

@rsashank Thanks for the moving the previous PR forward 👍

This certainly addresses some of the earlier points in that PR, though I did note that you dropped one change when comparing to it.

This turned out to be more work than I anticipated to complete fully, since we use various different names around the repo, including at least:

zulip_feature_level (server field name)
ZFL (abbreviation)
server_feature_level (in model, to distinguish from client)
feature_level (short variable, in tests)

That variation in names, and that they're not all code, certainly hasn't helped. Grepping through the repo, it seems there are a few outstanding unchanged parts remaining.

When you rebase and push this commit again, I suspect this will fail gitlint locally or in CI. A few other notes in that direction:

Fixes lines are good near the bottom after a blank line
This updates more than just the model, but you don't need to mention tests in the commit title (it's assumed they're always updated where necessary)
Since this builds upon the other PR it would be good to add a co-authored-by field near the bottom of the commit (see git log|grep -i co-au for examples)
For commit text, describing what the commit changes can be good, but just as important (if not more) is the reasoning (why is integer good? why is removing None good? what did None represent? when is ZFL absent?)

neiljp · 2024-01-27T00:17:38Z

tests/model/test_model.py

@@ -278,7 +278,7 @@ def test_register_initial_desired_events(self, mocker, initial_data):
                None,
                10,
                {1: "Indefinite [Organization default]"},
-                id="ZFL=None_no_stream_retention_realm_retention=None",
+                id="ZFL=10_no_stream_retention_realm_retention=None",


The None->10 change looks appropriate for the test case 👍

My concern here is that the text (id) here previously indicated 'ZFL=None', when it clearly is not. That may have been a typo, or indicative that we should have test case for the ZFL=0 case.

Added the ZFL=0 case with the same parameters as ZFL=10.

tests/model/test_model.py

zulipterminal/model.py

rsashank · 2024-01-27T18:29:19Z

tests/model/test_model.py

-            "remove_18_in_home_view:already_unmuted:ZFLNone",
-            "remove_19_in_home_view:muted:ZFLNone",
-            "add_19_in_home_view:already_muted:ZFLNone",
-            "add_30_in_home_view:unmuted:ZFLNone",
-            "remove_30_is_muted:already_unmutedZFL139",
+            "remove_18_in_home_view:already_unmuted:ZFL0",
+            "remove_19_in_home_view:muted:ZFL0",
+            "add_19_in_home_view:already_muted:ZFL0",
+            "add_30_in_home_view:unmuted:ZFL0",
+            "remove_30_is_muted:already_unmuted:ZFL139",


The absence of a ":" after already_unmuted seemed inconsistent with other ids. (line 3269)
While this change is unrelated to the commit/PR, it appears too minor to be a separate commit. Is that fine?

rsashank · 2024-01-27T18:35:38Z

tests/model/test_model.py

-            "remove_18_in_home_view:already_unmuted:ZFLNone",
-            "remove_19_in_home_view:muted:ZFLNone",
-            "add_19_in_home_view:already_muted:ZFLNone",
-            "add_30_in_home_view:unmuted:ZFLNone",
-            "remove_30_is_muted:already_unmutedZFL139",
+            "remove_18_in_home_view:already_unmuted:ZFL0",
+            "remove_19_in_home_view:muted:ZFL0",
+            "add_19_in_home_view:already_muted:ZFL0",
+            "add_30_in_home_view:unmuted:ZFL0",


I found these ones that weren't changed initially in the PR.

I'm still looking to identify any additional code that may need adjustments.

These were others I saw previously that were missing overall 👍

rsashank · 2024-01-27T18:44:54Z

Thanks for the detailed feedback! @neiljp

I've updated the PR, and I believe all of them have been addressed. I couldn't find any outliers, but please correct me if I'm mistaken. (Checked using VSCode, searched for ZFL and feature_level). I'll further check with grep as well.

Rebased and updated the commit message as per feedback as well.

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

mounilKshah

Thank you @rsashank for updating the PR
I noticed that you have implemented all the suggestions given by @neiljp

As Neil mentioned in the PR feedback, we have addressed ZFL with multiple phrases like feature level, Zulip feature level, ZFL etc. in code comments or in test case IDs. How about we also add another commit to this PR which renames them to the same name? What do you think of this idea @neiljp and @rsashank ?

rsashank · 2024-02-19T18:02:30Z

@mounilKshah That makes sense but what's the best way to ensure uniformity in variable names?

Would simply adding _ZFL to the end of variables where needed be sufficient (so you can grep through the repo with ZFL), or should we consider a more uniform rewrite across the repo?

mounilKshah · 2024-02-21T19:04:26Z

I was referring to comments and test IDs to have a uniform naming pattern across the repository, variables need not be changed. Naming all instances as ZFL can confuse a new contributor about its meaning, so maybe we can use the entire phrase Zulip Feature Level.

rsashank · 2024-02-25T20:35:40Z

Oh my bad. Makes sense, @neiljp should I make another commit addressing this?

neiljp

@rsashank Thanks for the update 👍

This looks to have full coverage for the change, so I've just pushed back with a slightly adjusted commit message.

I'd like to merge this soon (once CI passes), since tests written in other PRs assume None is possible, so setting this new expectation would be useful!

For that reason I'd prefer to consider any followup on this refactor separately.

While abbreviations aren't great in general, expanding ZFL would make for very long test case ids or comments (and it is very common)
I think we could certainly expand some versions to a common form, eg. feature_level
Documenting what ZFL corresponds to would likely be well-placed in api_types.py? We shouldn't use it in docs without the expanded form first, or point to api_types perhaps.
We could rename ZFL/zulip_feature_level to reflect the server-provided name, ie. server_feature_level, but that would be less important in terms of making it easier to search for phrases; it may also be something we adjust if we namespace server and organization names, which has been discussed as a possible migration in the server already.

neiljp · 2024-02-27T21:23:44Z

tests/model/test_model.py

@@ -3449,7 +3456,7 @@ def test__handle_subscription_event_visual_notifications(
                99,
                [1001, 11, 12],
            ),
-            ({"op": "peer_remove", "stream_id": 2, "user_id": 12}, None, 2, [1001, 11]),
+            ({"op": "peer_remove", "stream_id": 2, "user_id": 12}, 0, 2, [1001, 11]),


I believe this was one removed line from the other PR I noticed.

neiljp · 2024-02-27T21:46:27Z

tests/model/test_model.py

-            "remove_18_in_home_view:already_unmuted:ZFLNone",
-            "remove_19_in_home_view:muted:ZFLNone",
-            "add_19_in_home_view:already_muted:ZFLNone",
-            "add_30_in_home_view:unmuted:ZFLNone",
-            "remove_30_is_muted:already_unmutedZFL139",
+            "remove_18_in_home_view:already_unmuted:ZFL0",
+            "remove_19_in_home_view:muted:ZFL0",
+            "add_19_in_home_view:already_muted:ZFL0",
+            "add_30_in_home_view:unmuted:ZFL0",


These were others I saw previously that were missing overall 👍

When the server originally did not provide `zulip_feature_level`, this was previously represented by a `None` value in `model.server_feature_level`. This replaces the current potential `None` value with a zero value, allowing a pure integer representation in the model. This enables simplification of conditional statements when handling different server versions. The feature-level is represented by various strings in the repository, including: - server_feature_level - zulip_feature_level - ZFL - feature_level Tests updated, including test ids. Additional test case added for realm retention. Fixes zulip#1364. Co-authored-by: Lucas.C.B <[email protected]>

zulipbot added size: L [Automatic label added by zulipbot] area: refactoring labels Jan 17, 2024

neiljp reviewed Jan 27, 2024

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jan 27, 2024

rsashank force-pushed the ZFLHandling branch 3 times, most recently from e42a6b8 to 373b6df Compare January 27, 2024 18:05

rsashank changed the title ~~refactor: model/tests: Simplify Zulip Feature Level Handling.~~ refactor: model: Simplify Zulip Feature Level Handling. Jan 27, 2024

rsashank changed the title ~~refactor: model: Simplify Zulip Feature Level Handling.~~ Simplify Zulip Feature Level Handling. Jan 27, 2024

rsashank commented Jan 27, 2024

View reviewed changes

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

mounilKshah reviewed Feb 17, 2024

View reviewed changes

neiljp force-pushed the ZFLHandling branch from 373b6df to 85a5386 Compare February 27, 2024 22:19

neiljp reviewed Feb 27, 2024

View reviewed changes

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 labels Feb 27, 2024

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

neiljp force-pushed the ZFLHandling branch from 85a5386 to ee51893 Compare February 27, 2024 23:03

neiljp merged commit b71aec2 into zulip:main Feb 27, 2024
10 checks passed

This was referenced Feb 27, 2024

Refactor: Zulip Feature Level simplified #1405

Closed

Display read receipts: list of people who've read the message #1287

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Zulip Feature Level Handling. #1462

Simplify Zulip Feature Level Handling. #1462

rsashank commented Jan 17, 2024 •

edited

Loading

zulipbot commented Jan 17, 2024

neiljp left a comment

neiljp Jan 27, 2024

rsashank Jan 27, 2024

rsashank Jan 27, 2024

rsashank Jan 27, 2024

neiljp Feb 27, 2024

rsashank commented Jan 27, 2024

mounilKshah left a comment

rsashank commented Feb 19, 2024

mounilKshah commented Feb 21, 2024

rsashank commented Feb 25, 2024

neiljp left a comment

neiljp Feb 27, 2024

neiljp Feb 27, 2024

Simplify Zulip Feature Level Handling. #1462

Simplify Zulip Feature Level Handling. #1462

Conversation

rsashank commented Jan 17, 2024 • edited 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

Visual changes

zulipbot commented Jan 17, 2024

neiljp left a comment

Choose a reason for hiding this comment

neiljp Jan 27, 2024

Choose a reason for hiding this comment

rsashank Jan 27, 2024

Choose a reason for hiding this comment

rsashank Jan 27, 2024

Choose a reason for hiding this comment

rsashank Jan 27, 2024

Choose a reason for hiding this comment

neiljp Feb 27, 2024

Choose a reason for hiding this comment

rsashank commented Jan 27, 2024

mounilKshah left a comment

Choose a reason for hiding this comment

rsashank commented Feb 19, 2024

mounilKshah commented Feb 21, 2024

rsashank commented Feb 25, 2024

neiljp left a comment

Choose a reason for hiding this comment

neiljp Feb 27, 2024

Choose a reason for hiding this comment

neiljp Feb 27, 2024

Choose a reason for hiding this comment

rsashank commented Jan 17, 2024 •

edited

Loading