internal_link: Recognize channel operator in narrow links #704

Khader-1 · 2024-05-24T16:39:26Z

Fixes: #632

Khader-1

I feel like we don't need all those tests duplicated just to test that channel and stream can be used interchangeably! But I'm not sure. how could it be handled otherwise?

gnprice · 2024-05-24T18:22:26Z

Thanks @Khader-1!

Let's use the "buddy review" system here, now that the start of the official GSoC period is imminent. That means:

When you send a new PR, please request review from a buddy:
- […] So to request buddy review, please:
  - Pick one of your two buddies, and request review following the instructions in that doc.
- Add the "buddy review" label, to help make the state clear.
Conversely, when you get a buddy review request, please prioritize it right after important bugs and your regressions, ahead of most of your own PRs. (In particular I don't think anyone is currently working on an issue that would come ahead of doing buddy reviews.)

rajveermalviya

Thanks!
Looks great, just some comments related to style nits.

rajveermalviya · 2024-05-30T11:22:08Z

test/model/internal_link_test.dart

@@ -130,13 +130,16 @@ void main() {
      final bool expected = testCase.$1;
      final String description = testCase.$2;
      final String urlString = testCase.$3;
+      final String urlWithChannelSyntax = urlString.replaceFirst('#narrow/stream/', '#narrow/channel/');


nit: Feels like this can go unnoticed when skimming through tests, instead I'd just add those extra cases in the testCases list.

rajveermalviya · 2024-05-30T11:26:27Z

test/model/internal_link_test.dart

@@ -195,6 +219,8 @@ void main() {
      final testCases = [
        ('/#narrow/stream/name/topic/',           null), // missing operand
        ('/#narrow/stream/name/unknown/operand/', null), // unknown operator
+        ('/#narrow/channel/name/topic/',           null), // missing operand


nit:
I'd prefer a newline here to better segregate stream vs channel.

rajveermalviya · 2024-05-30T11:29:36Z

test/model/internal_link_test.dart

    });

    group('on stream link with wrong name, ID wins', () {
      testExpectedStreamNarrow('#narrow/stream/1-nonsense',        1);
      testExpectedStreamNarrow('#narrow/stream/1-',                1);
+      testExpectedStreamNarrow('#narrow/channel/1-nonsense',        1);


nit: newline, here and other similar cases below

rajveermalviya · 2024-05-30T11:33:23Z

test/model/internal_link_test.dart

        }
        final testCases = [
-          (mkUrlString('(no.20topic)'), TopicNarrow(stream.streamId, '(no topic)')),
-          (mkUrlString('lunch'),        TopicNarrow(stream.streamId, 'lunch')),
+          (mkUrlString('(no.20topic)', 'stream'), TopicNarrow(stream.streamId, '(no topic)')),


nit: Add whitespace to preserve formatting, here and similar cases below

(mkUrlString('(no.20topic)', 'stream'), TopicNarrow(stream.streamId, '(no topic)')), (mkUrlString('lunch', 'stream'), TopicNarrow(stream.streamId, 'lunch')), (mkUrlString('(no.20topic)', 'channel'), TopicNarrow(stream.streamId, '(no topic)')), (mkUrlString('lunch', 'channel'), TopicNarrow(stream.streamId, 'lunch')),

gnprice · 2024-05-31T03:32:18Z

I feel like we don't need all those tests duplicated just to test that channel and stream can be used interchangeably! But I'm not sure. how could it be handled otherwise?

Good question. I agree it'd be best not to duplicate most of these — after this changeover, we'd like to mostly stop thinking about the old forms, and have our code all focused on the "channel" forms. (Though not entirely stop thinking about the old forms, because it's important they continue to work.)

I think a good strategy would be to augment testExpectedNarrows so that it does the substitution to turn one kind of URL into the other and tests both ways. Then maybe have a handful of the test cases be explicitly duplicated just for good measure.

The main commit could have the tests transform "stream" to "channel", so that the change to the test file is small and easy to review. Then a followup commit, within the PR, can sweep through all the test URLs and make them say "/channel/" instead of "/stream/", and flip the transform around.

Khader-1 · 2024-05-31T10:12:51Z

Thanks for the review @rajveermalviya! and Thanks for the reply @gnprice!

How about this, keeping in mind that I'm already working on #631. We can take this approach:

Augment testExpectedNarrows to test both channel and stream syntax as mentioned by @gnprice.
Duplicate other test cases that do not use testExpectedNarrows to handle both cases.
Finally while working on Rename "stream" within codebase to "channel" #631 we'll reference stream as channel in test-cases that use testExpectedNarrows

I've already pushed a revision with the first two steps, please take a look.

rajveermalviya

Thanks for the revision! Apart from one comment, it looks great :)

Now over to @kenclary for mentor review.

rajveermalviya · 2024-06-01T08:41:43Z

test/model/internal_link_test.dart

+      // This is here to avoid duplicating each test case that contains `#narrow/stream` or `#narrow/channel`
+      final urlWithChannelSyntax = urlString.replaceFirst('#narrow/stream', '#narrow/channel');
+      final urlWithStreamSyntax = urlString.replaceFirst('#narrow/channel', '#narrow/stream');
+      for (final urlString in {urlWithStreamSyntax, urlWithChannelSyntax}) {


Suggested change

for (final urlString in {urlWithStreamSyntax, urlWithChannelSyntax}) {

for (final urlString in [urlWithStreamSyntax, urlWithChannelSyntax]) {

{'hello'} creates a Set<String>, if that's unintentional use [ ] instead, which creates List<String>

No it is actually intentional. Specifically, when the url being tested does not contain #narrow/stream nor #narrow/channel then both urlWithStreamSyntax, urlWithChannelSyntax would be the same and using the list would cause unnecessary duplications.

that sounds correct. it's obscure, though. can it get a comment explaining how the object is better than an array?

Sure, a new revision is there now.

gnprice · 2024-06-03T06:34:31Z

How about this, keeping in mind that I'm already working on #631. We can take this approach:

Sure, it's OK if the followup commit I mentioned at the end of #704 (comment) happens in a later PR as part of #631, instead of this PR.

I liked what the previous revision was doing to deduplicate the test cases that come from the big list of tuples, though, too. So what I had in mind was to deduplicate callers of testExpectedNarrows in addition to those.

For explicitly duplicated test cases, I really do mean just a handful — for example one boring StreamNarrow and one boring TopicNarrow, plus a TopicNarrow with /near/.

Khader-1 · 2024-06-09T05:40:38Z

Thanks @gnprice! Please check the new revision

chrisbobbe

Thanks! One small comment below.

chrisbobbe · 2024-06-12T19:03:07Z

test/model/internal_link_test.dart

+  final urlWithChannelSyntax = urlString.replaceFirst('#narrow/stream', '#narrow/channel');
+  final urlWithStreamSyntax = urlString.replaceFirst('#narrow/channel', '#narrow/stream');


Let's relax this so it handles the stream/channel operator even if it's at some position other than the first. It's usually the first, but it doesn't have to be. For example, this is a valid narrow link that the web app knows how to handle: https://chat.zulip.org/#narrow/is/unread/stream/14-GSoC

I think this could be done like this:

in urlString, skip past the #narrow/ at the beginning

split the rest by '/'

in that list of segments, look at the even-indexed ones (i.e., index % 2 == 0)

for each of those that's equal to "stream", replace it with "channel" and vice versa

(We wouldn't want to just replace all occurrences of "stream" or "channel" in the URL; we want to avoid doing things like changing a topic operand that happens to have one of those strings in it.)

It's true those are valid links, but it looks like we don't currently have any examples of them in this test file.

(Likely we should! Our implementation looks like it accepts them. But we don't right now. These tests were basically ported from zulip-mobile, because the tests there for this area were quite thorough, but that implementation doesn't accept those links.)

So for this PR, I'm happy to keep it simple in this respect.

Adding that logic would be a good followup, along with adding some test cases where /channel/ comes after /topic/, and where things come in other surprising orders.

Cool, thanks for that suggestion, Greg. Since this was my only comment, I'll mark this for integration review. 🙂

chrisbobbe · 2024-06-12T19:33:27Z

Marking for Greg's review following #704 (comment) ; @Khader-1 please take a look at that comment about a followup. 🙂

Fixes: zulip#632

gnprice · 2024-06-12T19:44:12Z

Thanks @Khader-1, and thanks @rajveermalviya and everyone for the reviews!

This looks good — merging. (See comment above about a useful followup.)

Khader-1 commented May 24, 2024

View reviewed changes

Khader-1 requested review from sm-sayedi and rajveermalviya and removed request for sm-sayedi May 28, 2024 09:13

Khader-1 added the buddy review GSoC buddy review needed. label May 28, 2024

Khader-1 force-pushed the recognize-channel-in-narrow-links branch from 66efd39 to 6a49c8c Compare May 29, 2024 22:13

rajveermalviya reviewed May 30, 2024

View reviewed changes

Khader-1 force-pushed the recognize-channel-in-narrow-links branch from 6a49c8c to b1a421e Compare May 31, 2024 10:02

rajveermalviya approved these changes Jun 1, 2024

View reviewed changes

rajveermalviya added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jun 1, 2024

rajveermalviya requested a review from kenclary June 1, 2024 08:45

Khader-1 force-pushed the recognize-channel-in-narrow-links branch from b1a421e to a55dedc Compare June 2, 2024 03:59

Khader-1 force-pushed the recognize-channel-in-narrow-links branch from a55dedc to 0728a6f Compare June 9, 2024 05:39

gnprice added the maintainer review PR ready for review by Zulip maintainers label Jun 10, 2024

gnprice requested a review from chrisbobbe June 10, 2024 07:00

chrisbobbe reviewed Jun 12, 2024

View reviewed changes

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 12, 2024

internal_link: Recognize channel operator in narrow links

50eba89

Fixes: zulip#632

gnprice force-pushed the recognize-channel-in-narrow-links branch from 0728a6f to 50eba89 Compare June 12, 2024 19:44

gnprice merged commit 50eba89 into zulip:main Jun 12, 2024
1 check passed

Khader-1 removed the mentor review GSoC mentor review needed. label Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal_link: Recognize channel operator in narrow links #704

internal_link: Recognize channel operator in narrow links #704

Khader-1 commented May 24, 2024 •

edited

Loading

Khader-1 left a comment

gnprice commented May 24, 2024

rajveermalviya left a comment

rajveermalviya May 30, 2024

rajveermalviya May 30, 2024

rajveermalviya May 30, 2024

rajveermalviya May 30, 2024

gnprice commented May 31, 2024

Khader-1 commented May 31, 2024

rajveermalviya left a comment

rajveermalviya Jun 1, 2024

Khader-1 Jun 1, 2024

kenclary Jun 1, 2024

Khader-1 Jun 2, 2024

gnprice commented Jun 3, 2024

Khader-1 commented Jun 9, 2024

chrisbobbe left a comment

chrisbobbe Jun 12, 2024 •

edited

Loading

gnprice Jun 12, 2024

chrisbobbe Jun 12, 2024

chrisbobbe commented Jun 12, 2024

gnprice commented Jun 12, 2024

	for (final urlString in {urlWithStreamSyntax, urlWithChannelSyntax}) {
	for (final urlString in [urlWithStreamSyntax, urlWithChannelSyntax]) {

		final urlWithChannelSyntax = urlString.replaceFirst('#narrow/stream', '#narrow/channel');
		final urlWithStreamSyntax = urlString.replaceFirst('#narrow/channel', '#narrow/stream');

internal_link: Recognize channel operator in narrow links #704

internal_link: Recognize channel operator in narrow links #704

Conversation

Khader-1 commented May 24, 2024 • edited Loading

Khader-1 left a comment

Choose a reason for hiding this comment

gnprice commented May 24, 2024

rajveermalviya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented May 31, 2024

Khader-1 commented May 31, 2024

rajveermalviya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Jun 3, 2024

Khader-1 commented Jun 9, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 12, 2024

gnprice commented Jun 12, 2024

Khader-1 commented May 24, 2024 •

edited

Loading

chrisbobbe Jun 12, 2024 •

edited

Loading