Integrate internal links in message contents #318
Conversation
test/widgets/content_test.dart
Outdated
| final connection = store.connection as FakeApiConnection; | ||
| connection.prepare(json: newestResult( | ||
| foundOldest: true, | ||
| messages: [], | ||
| ).toJson()); |
There was a problem hiding this comment.
What's the function of this part? It doesn't seem to be needed in the otherwise similar prepareContent in the previous group.
test/widgets/content_test.dart
Outdated
| final testNavObserver = TestNavigatorObserver() | ||
| ..onPushed = (route, prevRoute) => pushedRoutes.add(route); | ||
| await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot( | ||
| streams: [eg.stream(streamId: 1, name: 'check')], |
There was a problem hiding this comment.
What's the function of this line? Seems like the tests pass without it.
There was a problem hiding this comment.
Along with this and the previous comment I've pared down the setup. The connection.prepare wasn't necessary at all, but I think I arrived at this incrementally by adding things until the tests stopped complaining about things unrelated to the test I was writing. After further digging and consulting the other tests I've also found out the reason for the double await tester.pump() (the first is for the global store and second is for the pre-account store?) and I think this understanding will help me arrive at better test scaffolding (as well as understanding the existing tests)
There was a problem hiding this comment.
Leaving this line in particular for now, but started a discussion here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/possibly.20extreneous.20test.20scaffolding
test/widgets/content_test.dart
Outdated
| @@ -158,6 +166,78 @@ void main() { | |||
| }); | |||
| }); | |||
|
|
|||
| group('Internal links', () { | |||
There was a problem hiding this comment.
The other groups in this test file are named for the corresponding classes for the pieces of content they test. This should follow that, to help keep the file organized.
E.g. it could be "LinkNode on internal links", if it's not combined into the existing "LinkNode interactions".
test/widgets/content_test.dart
Outdated
| @@ -158,6 +166,78 @@ void main() { | |||
| }); | |||
| }); | |||
|
|
|||
| group('Internal links', () { | |||
| Future<List<Route<dynamic>>> prepareContentWithNavigator(WidgetTester tester, { | |||
There was a problem hiding this comment.
This function name contrasts with others in the file by saying "with navigator". But it doesn't create any more of a navigator than those other functions called prepareContent — they each mount a MaterialApp widget, and that creates a Navigator, exactly like what happens in this function. So the name is misleading.
One thing this function does do is add a navigator observer. But that's a kind of observer, not a kind of navigator.
I think just prepareContent suffices as a name. The function is a local helper for this test group; so it prepares some content, and the specific set of bells and whistles involved in doing so is just whatever set this test group needs.
test/widgets/content_test.dart
Outdated
| testWidgets('internal links are resolved: StreamNarrow', (tester) async { | ||
| final pushedRoutes = await prepareContentWithNavigator(tester, | ||
| html: '<p><a href="/#narrow/stream/1-check">stream</a></p>'); | ||
|
|
||
| await tester.tap(find.text('stream')); | ||
| check(testBinding.takeLaunchUrlCalls()).isEmpty(); | ||
| check(pushedRoutes).single.isA<WidgetRoute>() | ||
| .page.isA<MessageListPage>() | ||
| .narrow.equals(const StreamNarrow(1)); | ||
| }); | ||
|
|
||
| testWidgets('internal links are resolved: TopicNarrow', (tester) async { | ||
| final pushedRoutes = await prepareContentWithNavigator(tester, | ||
| html: '<p><a href="/#narrow/stream/1-check/topic/my.20topic">topic</a></p>'); |
There was a problem hiding this comment.
Having several versions like this for different types of narrow is OK, but FYI I'd say they're of low value — we already have extensive tests of the parsing that leads to the different types of narrow, over in test/model/internal_link_test.dart (from #305). The key things to test here are (a) some internal link, and (b) some link that resembles an internal link but is invalid, because those are the cases that this code-under-test knows the difference between.
sirpengi
force-pushed
the
pr-integrate-internal-links
branch
2 times, most recently
from
c5b0509 to
1bb4c50
Compare
|
Pushed up changes based on comments but awaiting discussion on #318 (comment) |
sirpengi
force-pushed
the
pr-integrate-internal-links
branch
from
1bb4c50 to
876e60f
Compare
|
I believe all comments were handled now and the discussion resolved, but I will re-review this PR and ping when ready. |
sirpengi
force-pushed
the
pr-integrate-internal-links
branch
from
876e60f to
a53a4f3
Compare
Integrates [parseInternalLink] into content link nodes so that recognized narrows are navigated to within the app, rather than opened in a browser. Fixes: zulip#73
sirpengi
force-pushed
the
pr-integrate-internal-links
branch
from
a53a4f3 to
0847f10
Compare
|
@gnprice Okay, ready for a review again! |
|
Thanks @sirpengi for the revision! All looks good — merging. |
This PR is built on top of #305
Intercepts link navigation to run through
parseInternalLink(from #305) and redirects internal links to their respected narrows.Fixes: #73