Handle internal PM links #883

Ezio-Sarthak · 2021-01-21T10:32:47Z

This is a follow-up PR from #708. It adds support for handling internal PM links.

I highly appreciate the work of @preetmishra in #708, by which it became simpler and cleaner to work with the helper functions in general :)

Partially fixes #764.

zulipbot · 2021-01-21T10:32:52Z

Hello @Ezio-Sarthak, it seems like you have referenced #764 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

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

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.

Thank you for your contributions to Zulip!

neiljp

@Ezio-Sarthak I've given this a quick look-over, initially since it was button-related and I moved some tests around. I've not run it yet.

The 'link to this message' code does seem to generate the code you have, but narrow URLs are more flexible than this in general, and other formats may be present in messages due to that. In the webapp itself there are more URL types generated; we may want to check with the server what values to expect. A few others I've come across:

<id>-<bot-name> (eg. 2414-shiny-bot)
<id>-user<id> (eg. 2406-user2406)

The latter seems like a strange choice (ending with email prefix?), but it's definitely widespread; it's unclear to me why these formats exist in addition to a simple eg. 2406/2414-pm/group. Other than the server source we might search czo for reasons.

zulipterminal/ui_tools/buttons.py

neiljp · 2021-01-26T07:13:01Z

tests/ui_tools/test_buttons.py

@@ -96,6 +96,20 @@ def test__decode_stream_data(self, stream_data, expected_response):

        assert return_value == expected_response

+    @pytest.mark.parametrize('pm_data, expected_response', [
+            ('1,2-pm', dict(recipient_ids=[1, 2], type='pm')),
+            ('1,2-group', dict(recipient_ids=[1, 2], type='group')),


Is a 1-size group OK? Is pm a synonym for group?

Oops, that needs correction. Thanks for pointing out.
To clarify:

pm is for 2 people private conversation

group is for more than 2 people private conversation

In principle that sounds likely - but pm and group appear to work in both cases on czo!

neiljp

@Ezio-Sarthak There are some conflicts to be resolved and the TypedDict issue, but otherwise this needs to be broadened to include both non-near and near forms.

With the basic structure in place, we can consider the other forms as a follow-up since we haven't had feedback about this.

zulipterminal/ui_tools/buttons.py

zee-bit · 2021-04-15T14:17:47Z

zulipterminal/ui_tools/buttons.py

+
+        # Currently webapp uses `pm` and `group` suffix interchangeably.
+        # Treat more-than-2 pms to group pms to avoid confusion.
+        pm_type = 'pm' if len(recipient_ids_list) < 3 else 'group'


Are we defining pm and group properly here?
The URL doesn't include the current-user's user_id. So, if the length of recipient_ids_list is 1 the number of persons involved would be 2(i.e. current-user and another user). This should be called pm, right?
Similarly, if the length of recipient_ids_list is >= 2, then number of persons involved would be 1 + len(recipient_ids_list), and this should be called group?

Correct me if I am wrong, please. :)

You're right! To clarify, the currently exposed formats of url may or may not contain the current user's user_id (manually tested this locally).

I'll add a conditional check to confirm if recipient_ids contain user id, and then calculate length accordingly. That should clear that edge case.

Thanks for poiting that out :)

I think the URL will contain the current user's user_id only if the user is conversing with himself?
Or, were you able to discover some other edge-case for this while testing?

The Zuilp server has exposed many URL narrow formats as of now (maybe a potential caveat :) ). Try narrowing to a PM by adding your id to the list, you'd still end up narrowing. I discovered that and some more easter eggs during manual testing ;)

neiljp · 2021-07-15T06:36:04Z

@Ezio-Sarthak This seems like it would be a fairly straightforward PR to get in, though:

this needs rebasing over the black transition?
this doesn't always narrow to the right message - this may be due to this PR being at a state prior to a fix that we merged later?

preetmishra

@Ezio-Sarthak Thanks for following up on the internal links work. This works well locally and the changes look good too. 👍 I have left a few minor in-line comments.

preetmishra · 2021-07-18T16:47:34Z

zulipterminal/ui_tools/buttons.py

+        recipient_ids_list = list(map(int, recipient_ids.split(",")))
+
+        no_of_recipients = len(recipient_ids_list)
+        # Bump no. of recipients tp include current user_id if not already


preetmishra · 2021-07-18T16:48:56Z

zulipterminal/ui_tools/buttons.py

+        no_of_recipients = len(recipient_ids_list)
+        # Bump no. of recipients tp include current user_id if not already
+        # present
+        if user_id not in recipient_ids_list:


What are the cases in which user_id is included/not included?

Well, the server has some (strange exposed) URL formats, that includes only the id(s) of the recipient(s) other than the current user, e.g.:

narrow/pm-with/1-user1

narrow/pm-with/1-bot-name

Those are the ones I'm aware of as of now

The above ones are those that don't have user_id included. Below are ones that have user_id included:

narrow/pm-with/1,2-pm

narrow/pm-with/1,2-group

narrow/pm-with/1,2,3-pm

narrow/pm-with/1,2,3-group

I'll add a note here to refer to the URL formats below in any case 👍

preetmishra · 2021-07-18T16:50:16Z

zulipterminal/ui_tools/buttons.py

+        Returns a dict with PM type and IDs of PM recipients.
+        """
+        # Recipient ids (seperated by `,`) are of prime interest.
+        recipient_ids, *_ = encoded_pm_data.split("-")


Perhaps recipient_data or something similar? This way the latter could be recipient_ids.

preetmishra

@Ezio-Sarthak I am not sure whether your commits were pushed properly. You seem to have reacted positively to my previous comments but the changes aren't present. That said, this continues to work well. I have left some more in-line comments. 👍

preetmishra · 2021-07-24T14:47:49Z

tests/ui_tools/test_buttons.py

+            "pm_with_bot_exposed_format_1_ordinary",
+            "pm_with_bot_exposed_format_1_ambigous",
+        ],
+    )


There are too many params here in the parametrization. Perhaps a good time to use from pytest import param as case as used elsewhere in the source, so that it is easier to correlate ids with test cases. :)

preetmishra · 2021-07-24T15:01:19Z

tests/ui_tools/test_buttons.py

@@ -464,7 +521,7 @@ def test__decode_message_id(self, message_id, expected_return_value):
        ],
    )
    def test__parse_narrow_link(self, link, expected_parsed_link):
-        return_value = MessageLinkButton._parse_narrow_link(link)
+        return_value = MessageLinkButton._parse_narrow_link(link, 1)


It is not apparent what 1 is. Perhaps we should mandate keyword args with *.

preetmishra · 2021-07-24T15:13:38Z

zulipterminal/ui_tools/buttons.py

@@ -561,13 +574,29 @@ def _switch_narrow_to(self, parsed_link: ParsedNarrowLink) -> None:
                topic_name=parsed_link["topic_name"],
                contextual_message_id=parsed_link["message_id"],
            )
+        elif "pm-with:near" == narrow:


We should validate whether the PM links are correct like we do for streams.

preetmishra · 2021-07-24T15:15:03Z

zulipterminal/ui_tools/buttons.py

@@ -561,13 +574,29 @@ def _switch_narrow_to(self, parsed_link: ParsedNarrowLink) -> None:
                topic_name=parsed_link["topic_name"],
                contextual_message_id=parsed_link["message_id"],
            )
+        elif "pm-with:near" == narrow:
+            emails = list(


It would be cleaner to only keep the narrowing part in the method like other conditionals.

This commit adds the helpers: - `_patch_narrow_link` as a general method to patch validated data. This way, explicit patching of stream/pm data is ensured. - `_validate_stream_data` to validate stream data separately in the general `_validate_narrow_link`. Tests added.

This commit adds a static method that decodes necessary information from a PM narrow message link, i.e., the list of PM recipient ids. TypedDict `DecodedPM` is added to return the required structured format of PM data. Currently, there are a bunch of exposed url formats supported by the Zulip server, such as (some seem ambigous too?): narrow/pm-with/1,2-pm narrow/pm-with/1,2-group narrow/pm-with/1,2,3-pm narrow/pm-with/1,2,3-group narrow/pm-with/1-user1 narrow/pm-with/1-bot-name (near parameter supported for each) Tests added.

This commit adds conditional checks in `_parse_narrow_link` to detect PM narrow links. On successful detection, the narrow is switched to the respective PM message via `_switch_narrow_to`. NOTE: A PM narrow can be either "all PMs" (not near) or a "specific PM conversation" (near). Tests added and amended.

This commit adds validation helper for PM data, in an attempt to avoid and handle any potential exceptions before starting to switch to the PM narrow. The validation checks if all the recipients IDs are valid or not. Tests added and amended.

prah23 · 2021-08-01T20:44:08Z

tests/ui_tools/test_buttons.py

+        [
+            case(
+                "1001,12-pm",
+                dict(type=None, recipient_ids=[1001, 12], recipient_emails=None),


Since you type these expected_response cases as DecodedPM in the test parameters below, it would be more appropriate to have these as DecodedPM instances rather than dict instances (similar to how we have ParsedNarrowLink instances as parameter cases in the other tests in this file).

zulipbot · 2023-02-24T21:19:48Z

Heads up @Ezio-Sarthak, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: L [Automatic label added by zulipbot] label Jan 21, 2021

Ezio-Sarthak force-pushed the handle-internal-pm-links branch 2 times, most recently from e1aa68c to bb4d15a Compare January 23, 2021 06:39

neiljp reviewed Jan 26, 2021

View reviewed changes

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from bb4d15a to d9896cb Compare January 28, 2021 05:08

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:31

zulipbot added the has conflicts label Jan 30, 2021

neiljp reviewed Feb 4, 2021

View reviewed changes

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 4, 2021

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from d9896cb to f838f12 Compare March 26, 2021 17:02

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] has conflicts labels Mar 26, 2021

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from f838f12 to 3dc6001 Compare March 26, 2021 17:44

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 Mar 27, 2021

Ezio-Sarthak requested a review from neiljp March 27, 2021 14:36

Ezio-Sarthak force-pushed the handle-internal-pm-links branch 3 times, most recently from 6453d26 to 89afed5 Compare March 31, 2021 02:26

zee-bit reviewed Apr 15, 2021

View reviewed changes

Ezio-Sarthak force-pushed the handle-internal-pm-links branch 2 times, most recently from 7b03b4a to 7b23aa3 Compare April 17, 2021 03:32

Ezio-Sarthak force-pushed the handle-internal-pm-links branch 2 times, most recently from c766f8a to 126ab37 Compare July 15, 2021 15:22

preetmishra reviewed Jul 18, 2021

View reviewed changes

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from 126ab37 to 0704275 Compare July 18, 2021 17:52

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from 0704275 to e5c9387 Compare July 22, 2021 19:09

preetmishra reviewed Jul 24, 2021

View reviewed changes

preetmishra 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 Jul 24, 2021

Ezio-Sarthak force-pushed the handle-internal-pm-links branch 2 times, most recently from e370475 to b5e39d8 Compare August 1, 2021 08:52

refactor: tests: Use case for patching test ids of large frequency.

16b9e3e

Ezio-Sarthak force-pushed the handle-internal-pm-links branch 2 times, most recently from 031f42e to 0080312 Compare August 1, 2021 14:02

Ezio-Sarthak added 3 commits August 1, 2021 21:04

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from 0080312 to 6c7719b Compare August 1, 2021 15:35

Ezio-Sarthak force-pushed the handle-internal-pm-links branch from 6c7719b to b8fb00f Compare August 1, 2021 18:17

prah23 reviewed Aug 1, 2021

View reviewed changes

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

neiljp mentioned this pull request Apr 14, 2022

TRACKING: Handle message links #764

Open

4 tasks

neiljp modified the milestones: Next Release, 0.6.0, Release after next May 16, 2022

zulipbot added the has conflicts label Feb 24, 2023

Uh oh!

Handle internal PM links #883

Are you sure you want to change the base?

Handle internal PM links #883

Uh oh!

Conversation

Ezio-Sarthak commented Jan 21, 2021

Uh oh!

zulipbot commented Jan 21, 2021

Uh oh!

neiljp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ezio-Sarthak Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ezio-Sarthak Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Jul 15, 2021

Uh oh!

preetmishra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preetmishra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zulipbot commented Feb 24, 2023

Uh oh!

Uh oh!

neiljp left a comment •

edited

Loading

Ezio-Sarthak Jan 28, 2021 •

edited

Loading

Ezio-Sarthak Apr 16, 2021 •

edited

Loading