Migrated get_next_unread_pm and tests to model. #1395

WladRamos · 2023-05-06T20:01:44Z

What does this PR do, and why?

This pull request Migrates get_next_unread_pm & tests to model.
Created this new version to the pull request to make it cleaner by splitting the changes into two commits.
This is a precursor for improving the get_next_unread_pm method.
Fixes #1335

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes Migrate get_next_unread_pm & tests to model #1335
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 TRACKING: Improve algorithm for 'next unread direct message' (p) #1336

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

zulipbot · 2023-05-06T20:01:49Z

Hello @WladRamos, it seems like you have referenced #1335 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #1335..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

Update test accordingly, and improve similar test to match.

neiljp · 2023-05-10T03:12:11Z

@WladRamos I just merged a slightly trimmed version of your first commit, and the other parts back to this PR. I'm going to make some notes in a moment.

neiljp

@WladRamos Thanks for working on this 👍

Here are the comments for the followup commits.

neiljp · 2023-05-10T02:27:46Z

zulipterminal/model.py

@@ -902,6 +903,21 @@ def get_next_unread_topic(self) -> Optional[Tuple[int, str]]:
                next_topic = True
        return None

+    def get_next_unread_pm(self) -> Optional[int]:
+        pms = sorted(self.unread_counts["unread_pms"].keys())


This should stay unsorted for this commit.

neiljp · 2023-05-10T03:13:16Z

zulipterminal/ui_tools/views.py

@@ -616,6 +616,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
                recipient_emails=[email],
                contextual_message_id=pm,
            )
+            return key


This change is small, but I split it out from the first commit since it's a distraction from the main refactor.

neiljp · 2023-05-10T03:13:47Z

tests/ui/test_ui_tools.py

@@ -933,6 +934,7 @@ def test_keypress_NEXT_UNREAD_PM_no_pm(

        return_value = mid_col_view.keypress(size, key)

+        assert not mid_col_view.controller.narrow_to_user.called


Similarly this looks reasonable, but unrelated to that first commit (migration).

neiljp · 2023-05-10T03:16:47Z

tests/model/test_model.py

+        "unread_pms, last_unread_pm, next_unread_pm",
+        [
+            case(
+                {(1, "pm"), (2, "pm2")},


Note that unread_pms are, based on the unread_counts definition, int->int, from sender-id to unread count, so these strings are misleading.

neiljp · 2023-05-10T03:19:02Z

tests/model/test_model.py

+    def test_get_next_unread_pm(
+        self, model, unread_pms, last_unread_pm, next_unread_pm
+    ):
+        model.unread_counts = {"unread_pms": {stream_pm: 1 for stream_pm in unread_pms}}


What is stream_pm here?

neiljp · 2023-05-10T03:39:16Z

tests/model/test_model.py

+            ),
+            case(
+                {(1, "pm"), (2, "pm2")},
+                (1, "pm"),


Similarly in the test refactor here, this is setting model.last_unread_pm to a tuple, which is different.

neiljp · 2023-05-10T03:40:22Z

tests/model/test_model.py

+
+        unread_pm = model.get_next_unread_pm()
+
+        assert unread_pm == next_unread_pm


Is this missing an assert?

zulipbot · 2023-05-10T03:41:36Z

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

WladRamos · 2023-05-10T22:59:01Z

@neiljp Thanks for the feedback, I'll take a look. 👍

neiljp · 2024-05-29T00:18:16Z

As noted in #1335, the migration was complete in the merged commit, but this remains a useful refactor as part of #1336, so marking this as a PR completion candidate.

zulipbot added the size: L [Automatic label added by zulipbot] label May 6, 2023

WladRamos mentioned this pull request May 6, 2023

Migrated get_next_unread_pm & tests to model. Fixes #1335 #1392

Closed

18 tasks

WladRamos added 2 commits May 9, 2023 20:06

views: Return key after get_next_unread_pm.

56ec47d

Update test accordingly, and improve similar test to match.

refactor: tests: model: Combine get_next_unread_pm tests.

647e77f

neiljp force-pushed the issue#1335 branch from b0b91a0 to 647e77f Compare May 10, 2023 03:11

zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels May 10, 2023

neiljp reviewed May 10, 2023

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback area: tests area: refactoring labels May 10, 2023

zormit mentioned this pull request May 20, 2024

Migrate get_next_unread_pm & tests to model #1335

Closed

neiljp added PR completion candidate PR with reviews that may unblock merging and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels May 29, 2024

neiljp mentioned this pull request May 29, 2024

TRACKING: Improve algorithm for 'next unread direct message' (p) #1336

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Migrated get_next_unread_pm and tests to model. #1395

Migrated get_next_unread_pm and tests to model. #1395

Uh oh!

WladRamos commented May 6, 2023

Uh oh!

zulipbot commented May 6, 2023

Uh oh!

neiljp commented May 10, 2023

Uh oh!

neiljp left a comment

Uh oh!

neiljp May 10, 2023

Uh oh!

neiljp May 10, 2023

Uh oh!

neiljp May 10, 2023

Uh oh!

neiljp May 10, 2023

Uh oh!

neiljp May 10, 2023

Uh oh!

neiljp May 10, 2023

Uh oh!

neiljp May 10, 2023

Uh oh!

zulipbot commented May 10, 2023

Uh oh!

WladRamos commented May 10, 2023 •

edited

Loading

Uh oh!

neiljp commented May 29, 2024

Uh oh!

Uh oh!

		@@ -933,6 +934,7 @@ def test_keypress_NEXT_UNREAD_PM_no_pm(

		return_value = mid_col_view.keypress(size, key)

		assert not mid_col_view.controller.narrow_to_user.called


		unread_pm = model.get_next_unread_pm()

		assert unread_pm == next_unread_pm

Uh oh!

Migrated get_next_unread_pm and tests to model. #1395

Are you sure you want to change the base?

Migrated get_next_unread_pm and tests to model. #1395

Uh oh!

Conversation

WladRamos commented May 6, 2023

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Uh oh!

zulipbot commented May 6, 2023

Uh oh!

neiljp commented May 10, 2023

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

neiljp May 10, 2023

Choose a reason for hiding this comment

Uh oh!

zulipbot commented May 10, 2023

Uh oh!

WladRamos commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented May 29, 2024

Uh oh!

Uh oh!

WladRamos commented May 10, 2023 •

edited

Loading