-
Notifications
You must be signed in to change notification settings - Fork 378
internal_links: Add "/" before fragment identifier #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -96,7 +96,11 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { | |||||||||
| fragment.write('/near/$nearMessageId'); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return store.realmUrl.replace(fragment: fragment.toString()); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 96593ba
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have tests in this commit, and the tests added in the other commit do not fail if we remove this change. We need to do some testing here. |
||||||||||
| final realmUrlWithSlash = store.realmUrl.path.isNotEmpty | ||||||||||
| ? store.realmUrl | ||||||||||
| : store.realmUrl.replace(path: '${store.realmUrl.path}/'); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
Comment on lines
+99
to
+101
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the "else" case here, the path will have to be empty. So this expression is unnecessarily complicated. |
||||||||||
|
|
||||||||||
| return realmUrlWithSlash.replace(fragment: fragment.toString()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// A [Narrow] from a given URL, on `store`'s realm. | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -38,11 +38,14 @@ late FakeApiConnection connection; | |||||||||||||||||
| Future<void> setupToMessageActionSheet(WidgetTester tester, { | ||||||||||||||||||
| required Message message, | ||||||||||||||||||
| required Narrow narrow, | ||||||||||||||||||
| Account? account, | ||||||||||||||||||
| }) async { | ||||||||||||||||||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add parameter Account so that we can override realmUrl to produce Url without '/' before '#'. |
||||||||||||||||||
| addTearDown(testBinding.reset); | ||||||||||||||||||
|
|
||||||||||||||||||
| await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); | ||||||||||||||||||
| final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); | ||||||||||||||||||
| account ??= eg.selfAccount; | ||||||||||||||||||
| final initialSnapshot = eg.initialSnapshot(); | ||||||||||||||||||
| await testBinding.globalStore.add(account, initialSnapshot); | ||||||||||||||||||
| final store = await testBinding.globalStore.perAccount(account.id); | ||||||||||||||||||
| await store.addUser(eg.user(userId: message.senderId)); | ||||||||||||||||||
| if (message is StreamMessage) { | ||||||||||||||||||
| final stream = eg.stream(streamId: message.streamId); | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! There is one extra spot below where the |
||||||||||||||||||
|
|
@@ -61,7 +64,7 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, { | |||||||||||||||||
| messages: [message], | ||||||||||||||||||
| ).toJson()); | ||||||||||||||||||
|
|
||||||||||||||||||
| await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, | ||||||||||||||||||
| await tester.pumpWidget(TestZulipApp(accountId: account.id, | ||||||||||||||||||
| child: MessageListPage(initNarrow: narrow))); | ||||||||||||||||||
|
|
||||||||||||||||||
| // global store, per-account store, and message list get loaded | ||||||||||||||||||
|
|
@@ -553,6 +556,82 @@ void main() { | |||||||||||||||||
| }); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| group('Message link verbatim URL tests', () { | ||||||||||||||||||
| Future<void> tapCopyMessageLinkButton(WidgetTester tester) async { | ||||||||||||||||||
| await tester.ensureVisible(find.byIcon(Icons.link, skipOffstage: false)); | ||||||||||||||||||
| await tester.tap(find.byIcon(Icons.link)); | ||||||||||||||||||
| await tester.pump(); // [MenuItemButton.onPressed] called in a post-frame callback: flutter/flutter@e4a39fa2e | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Future<void> testMessageLink({ | ||||||||||||||||||
| required WidgetTester tester, | ||||||||||||||||||
| required String realmUrl, | ||||||||||||||||||
| required String topic, | ||||||||||||||||||
| required String expectedLink, | ||||||||||||||||||
| }) async { | ||||||||||||||||||
| final stream = eg.stream(streamId: 42, name: 'stream 42'); | ||||||||||||||||||
| final message = eg.streamMessage(id: 12345, stream: stream, topic: topic); | ||||||||||||||||||
| final narrow = TopicNarrow.ofMessage(message); | ||||||||||||||||||
| final account = eg.selfAccount.copyWith(realmUrl: Uri.parse(realmUrl)); | ||||||||||||||||||
| await setupToMessageActionSheet( | ||||||||||||||||||
| tester, message: message, narrow: narrow, account: account); | ||||||||||||||||||
|
|
||||||||||||||||||
| final store = await testBinding.globalStore.perAccount(account.id); | ||||||||||||||||||
| if (!store.streams.containsKey(stream.streamId)) { | ||||||||||||||||||
| await store.addStream(stream); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| await tapCopyMessageLinkButton(tester); | ||||||||||||||||||
| await tester.pump(Duration.zero); | ||||||||||||||||||
|
Comment on lines
+584
to
+585
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Move this to just before the line where we define |
||||||||||||||||||
| check(await Clipboard.getData('text/plain')).isNotNull().text.equals(expectedLink); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| testWidgets('Copies link with custom realm path', (tester) async { | ||||||||||||||||||
| await testMessageLink( | ||||||||||||||||||
| tester: tester, | ||||||||||||||||||
| realmUrl: 'https://chat.example/foo', | ||||||||||||||||||
|
Comment on lines
+589
to
+592
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is difficult to read because it's missing indentation:
Suggested change
The same is true of each of the other test cases. |
||||||||||||||||||
| topic: 'test topic', | ||||||||||||||||||
| expectedLink: 'https://chat.example/foo#narrow/stream/42-stream-42/topic/test.20topic/near/12345', | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| testWidgets('Copies link with realm URL without path', (tester) async { | ||||||||||||||||||
| await testMessageLink( | ||||||||||||||||||
| tester: tester, | ||||||||||||||||||
| realmUrl: 'https://chat.zulip.org', | ||||||||||||||||||
| topic: 'test topic', | ||||||||||||||||||
| expectedLink: 'https://chat.zulip.org/#narrow/stream/42-stream-42/topic/test.20topic/near/12345', | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| testWidgets('Copies link with special characters in topic', (tester) async { | ||||||||||||||||||
| await testMessageLink( | ||||||||||||||||||
| tester: tester, | ||||||||||||||||||
| realmUrl: 'https://chat.zulip.org', | ||||||||||||||||||
| topic: 'What\'s up? 123!', | ||||||||||||||||||
| expectedLink: 'https://chat.zulip.org/#narrow/stream/42-stream-42/topic/What\'s.20up.3F.20123!/near/12345', | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| testWidgets('Copies link with realm URL ending in path', (tester) async { | ||||||||||||||||||
| await testMessageLink( | ||||||||||||||||||
| tester: tester, | ||||||||||||||||||
| realmUrl: 'https://chat.zulip.org/path', | ||||||||||||||||||
| topic: 'Numbers & Symbols: 100%', | ||||||||||||||||||
| expectedLink: 'https://chat.zulip.org/path#narrow/stream/42-stream-42/topic/Numbers.20.26.20Symbols.3A.20100.25/near/12345', | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| testWidgets('Copies link with realm URL ending in slash', (tester) async { | ||||||||||||||||||
| await testMessageLink( | ||||||||||||||||||
| tester: tester, | ||||||||||||||||||
| realmUrl: 'https://chat.zulip.org/', | ||||||||||||||||||
| topic: 'Regular topic', | ||||||||||||||||||
| expectedLink: 'https://chat.zulip.org/#narrow/stream/42-stream-42/topic/Regular.20topic/near/12345', | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| group('ShareButton', () { | ||||||||||||||||||
| // Tests should call this. | ||||||||||||||||||
| MockSharePlus setupMockSharePlus() { | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I missed this in the last round. "Variable" in the commit summary is capitalized, but we should use "variable" instead.