internal_link: Always include a "/" after hostname #1059

PIG208 · 2024-11-13T19:51:37Z

Meanwhile, reorganized some test groups with the same name.

Supersedes: #874
Fixes: #845

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2024-11-22T03:59:19Z

lib/model/internal_link.dart

+    // A generated URL without '/' looks odd and does not linkify.
+    result = result.replace(path: '/');
+  }
+  assert(result.path.startsWith('/'));


I think we can leave out the assert; it's redundant with tests and doesn't open opportunities to simplify code below it.

Makes sense. This feels like a duplication of the assertion made in the comments above.

chrisbobbe · 2024-11-22T04:05:11Z

test/model/compose_test.dart

+        check(narrowLink(store, const CombinedFeedNarrow()))
+          .equals(Uri.parse(expectedLink));


I'm slightly inclined to stringify the narrowLink result and compare that to the String expectedLink. The user's experience of a narrowLink result is always through Uri.toString. (Quote-and-reply and copy-link-to-message.)

PIG208 · 2024-11-22T16:46:49Z

Thanks for the review! I have updated the PR.

chrisbobbe · 2024-11-25T21:12:33Z

Thanks, LGTM! Marking for Greg's review.

gnprice

Thanks @PIG208, and thanks @chrisbobbe for the previous review! Comments below.

gnprice · 2024-12-03T23:05:45Z

test/model/internal_link_test.dart

@@ -184,6 +184,36 @@ void main() {
      testExpectedNarrows(testCases, streams: streams);
    });

+    group('topic link parsing', () {


nit:

internal_link test [nfc]: Merge tests with the same name

s/tests/groups/ — this commit doesn't merge any tests (and it doesn't look like we had any tests with the same name).

gnprice · 2024-12-03T23:10:04Z

test/model/internal_link_test.dart

@@ -184,6 +184,36 @@ void main() {
      testExpectedNarrows(testCases, streams: streams);
    });

+    group('topic link parsing', () {


Moving these tests up here splits some related tests from each other: the tests above and below this moved group all use the same streams local and have the same structure. So best to keep those together by leaving these below them.

Or in fact let's just leave these where they are entirely — it looks like you don't end up making any changes to this test file in the main commit, so it doesn't seem like in this PR we're making use of any energy we spend loading these tests into our heads enough to refactor them.

… I guess that last remark is made moot by my other comment that the new tests should go in this file.

I'll push a revision with a new commit that fixes these group-name collisions more minimally. We're still not in this PR doing anything with these tests of parseInternalLink — the code being modified isn't parseInternalLink, but rather narrowLink which is roughly its inverse — so we don't need to think too hard about these tests at the moment.

gnprice · 2024-12-03T23:18:27Z

lib/model/internal_link.dart

+  Uri result = store.realmUrl.replace(fragment: fragment.toString());
+  if (result.path.isEmpty) {
+    // Always ensure that there is a '/' right after the hostname.
+    // A generated URL without '/' looks odd and does not linkify.


Suggested change

// A generated URL without '/' looks odd and does not linkify.

// A generated URL without '/' looks odd,

// and if used in a Zulip message does not get automatically linkified.

The not getting linkified is a specific fact about what Zulip does with URLs it spots in the plain text of a message; it's not a general fact about URLs, and this function isn't specific to generating something to go into a Zulip message.

gnprice · 2024-12-03T23:22:38Z

test/model/compose_test.dart

@@ -314,6 +314,28 @@ hello
        '#narrow/dm/1,2-dm/near/12345',
        '#narrow/pm-with/1,2-pm/near/12345');
    });
+
+    test('normalize links to always include a "/" after hostname', () {


The code this is testing is in internal_link.dart, not compose.dart, so let's place the test to match. We can do that with a prep commit moving this narrowLink test group to the top of internal_link_test.dart; really that should have happened as part of 2f944a0.

gnprice · 2024-12-03T23:24:55Z

test/model/compose_test.dart

+        final account = eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl));
+        final store = eg.store(account: account);


nit: just make a fresh Account from scratch:

Suggested change

final account = eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl));

final store = eg.store(account: account);

final store = eg.store(account: eg.account(realmUrl: Uri.parse(realmUrl)));

Or is there a reason that doesn't work?

It would be:

final store = eg.store(account: eg.account(user: eg.selfUser, realmUrl: Uri.parse(realmUrl)));

which is fine and actually as short as the other way:

final store = eg.store(account: eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl)));

... Oh, I didn't see this reply when writing #1059 (comment) . I think I got got by the GitHub bug where a PR page often/usually live-updates with new comments, but then often doesn't, so I was looking at the page (in an existing tab) and didn't see any reply.

Anyway, if it needs that extra detail, no need to change it. And from your revision I see there's an existing test there that sets this up the same way.

I guess the non-copy-with implementation would be a tiny bit more realistic because we supposedly (there is a TODO on eg.account) will regenerate a bunch of fields from scratch as well.

Hmm true. Not a bunch, I don't think; but the account ID, and maybe the API key too. Sure.

gnprice · 2024-12-03T23:30:21Z

test/model/compose_test.dart

+      checkGeneratedLink(
+        realmUrl:     'http://chat.example.com',
+        expectedLink: 'http://chat.example.com/#narrow');


nit: always best to make the code transparent about what it's doing, when that doesn't come at a high cost in concision:

Suggested change

checkGeneratedLink(

realmUrl: 'http://chat.example.com',

expectedLink: 'http://chat.example.com/#narrow');

check(generatedLink('http://chat.example.com'))

.equals( 'http://chat.example.com/#narrow');

(And here it actually improves concision, too).

Then further can rename generatedLink to something a bit more specific, like narrowLinkFor.

The organization of this code's test cases is a bit scattered; partly because they were ported from tests written for the legacy zulip-mobile RN app, which themselves accumulated in layers over time. (After all, one doesn't want to drop one of these tests in a refactoring unless taking the time to be sure its substance is covered by a surviving test.) It's potentially confusing having several groups with the same name, though. Fix that by picking different names for all but one of them. The name collision was originally noted by Zixuan in zulip#1059.

PIG208 · 2024-12-11T19:39:49Z

Thanks for the commit and reviews! I have updated the PR.

gnprice

Thanks for the revision! Just one surviving comment.

Oh and in a commit message:

5a1caac internal_link test [nfc]: Give test groups distinct names
5acef65 internal_link test [nfc]: Re-home narrow link tests
1ff2621 internal_links: Always include a "/" after hostname

There's no identifier internal_links, so that identifier-styled term shouldn't appear in text when there isn't an identifier it refers to.

gnprice · 2024-12-11T22:53:25Z

test/model/internal_link_test.dart

+
+    test('normalize links to always include a "/" after hostname', () {
+      String narrowLinkFor({required String realmUrl}) {
+        final store = eg.store(account: eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl)));


bump #1059 (comment)

The organization of this code's test cases is a bit scattered; partly because they were ported from tests written for the legacy zulip-mobile RN app, which themselves accumulated in layers over time. (After all, one doesn't want to drop one of these tests in a refactoring unless taking the time to be sure its substance is covered by a surviving test.) It's potentially confusing having several groups with the same name, though. Fix that by picking different names for all but one of them. The name collision was originally noted by Zixuan in zulip#1059.

gnprice · 2024-12-11T23:23:06Z

Thanks! Merging, after the subthread at #1059 (comment) .

The organization of this code's test cases is a bit scattered; partly because they were ported from tests written for the legacy zulip-mobile RN app, which themselves accumulated in layers over time. (After all, one doesn't want to drop one of these tests in a refactoring unless taking the time to be sure its substance is covered by a surviving test.) It's potentially confusing having several groups with the same name, though. Fix that by picking different names for all but one of them. The name collision was originally noted by Zixuan in zulip#1059.

Signed-off-by: Zixuan James Li <[email protected]>

Fixes: zulip#845 Signed-off-by: Zixuan James Li <[email protected]>

The organization of this code's test cases is a bit scattered; partly because they were ported from tests written for the legacy zulip-mobile RN app, which themselves accumulated in layers over time. (After all, one doesn't want to drop one of these tests in a refactoring unless taking the time to be sure its substance is covered by a surviving test.) It's potentially confusing having several groups with the same name, though. Fix that by picking different names for all but one of them. The name collision was originally noted by Zixuan in zulip#1059.

PIG208 requested a review from chrisbobbe November 13, 2024 19:51

PIG208 assigned chrisbobbe Nov 13, 2024

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Nov 18, 2024

chrisbobbe reviewed Nov 22, 2024

View reviewed changes

PIG208 force-pushed the pr-links branch from 05bb739 to 49ce514 Compare November 22, 2024 16:46

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Nov 25, 2024

chrisbobbe requested a review from gnprice November 25, 2024 21:12

chrisbobbe assigned gnprice and unassigned chrisbobbe Nov 25, 2024

gnprice reviewed Dec 3, 2024

View reviewed changes

gnprice force-pushed the pr-links branch from 49ce514 to a198c84 Compare December 3, 2024 23:46

PIG208 force-pushed the pr-links branch 2 times, most recently from f3f3f1f to 03c7c5a Compare December 11, 2024 19:39

PIG208 requested a review from gnprice December 11, 2024 19:39

PIG208 force-pushed the pr-links branch from 03c7c5a to 1ff2621 Compare December 11, 2024 19:41

gnprice reviewed Dec 11, 2024

View reviewed changes

PIG208 force-pushed the pr-links branch from 1ff2621 to afc4650 Compare December 11, 2024 23:01

PIG208 requested a review from gnprice December 11, 2024 23:01

gnprice and others added 4 commits December 11, 2024 15:23

internal_link test [nfc]: Re-home narrow link tests

6fff765

Signed-off-by: Zixuan James Li <[email protected]>

internal_link test [nfc]: Set up account from scratch

223d674

Signed-off-by: Zixuan James Li <[email protected]>

internal_link: Always include a "/" after hostname

fd91f72

Fixes: zulip#845 Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the pr-links branch from afc4650 to fd91f72 Compare December 11, 2024 23:25

gnprice merged commit fd91f72 into zulip:main Dec 11, 2024

PIG208 deleted the pr-links branch December 11, 2024 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal_link: Always include a "/" after hostname #1059

internal_link: Always include a "/" after hostname #1059

PIG208 commented Nov 13, 2024

chrisbobbe left a comment

chrisbobbe Nov 22, 2024

PIG208 Nov 22, 2024

chrisbobbe Nov 22, 2024

PIG208 commented Nov 22, 2024

chrisbobbe commented Nov 25, 2024

gnprice left a comment

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

PIG208 Dec 11, 2024

gnprice Dec 11, 2024

PIG208 Dec 11, 2024 •

edited

Loading

gnprice Dec 11, 2024

gnprice Dec 3, 2024

PIG208 commented Dec 11, 2024

gnprice left a comment

gnprice Dec 11, 2024

gnprice commented Dec 11, 2024

		check(narrowLink(store, const CombinedFeedNarrow()))
		.equals(Uri.parse(expectedLink));

	// A generated URL without '/' looks odd and does not linkify.
	// A generated URL without '/' looks odd,
	// and if used in a Zulip message does not get automatically linkified.

		final account = eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl));
		final store = eg.store(account: account);

	final account = eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl));
	final store = eg.store(account: account);
	final store = eg.store(account: eg.account(realmUrl: Uri.parse(realmUrl)));

internal_link: Always include a "/" after hostname #1059

internal_link: Always include a "/" after hostname #1059

Conversation

PIG208 commented Nov 13, 2024

chrisbobbe 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

PIG208 commented Nov 22, 2024

chrisbobbe commented Nov 25, 2024

gnprice 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

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

PIG208 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Dec 11, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Dec 11, 2024

PIG208 Dec 11, 2024 •

edited

Loading