Aman-Mittal commented on PR #270:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/270#issuecomment-5227134405
The change is right, and I checked it rather than eyeballing it. Applied
this exact diff locally against a running app:
| URL | Result |
|---|---|
| `/calendars` | → `/dashboard` |
| `/meetings` | → `/dashboard` |
| `/calendars/groups/1` | stays put, `app-calendars-list` mounts |
| `/meetings/groups/1` | stays put |
The last two are the ones worth knowing about, because they are the
regression this could plausibly have caused and nothing in the PR covers them.
`pathMatch: 'full'` is doing its job — thank you for including it, an
unqualified `redirectTo` here would have swallowed the entity-scoped routes and
that failure would not have been obvious.
## One thought on the specs
They currently assert that the route config object equals the literal
written in the source file:
```ts
const rootRoute = CALENDARS_ROUTES.find(({ path }) => path === '');
expect(rootRoute).toEqual({ path: '', pathMatch: 'full', redirectTo:
'/dashboard' });
```
That guards against someone deleting the route by accident, which is worth
something. But it cannot fail for any reason other than the literal being
edited — it would pass just as happily if the redirect never worked at runtime,
so it does not really pin the behaviour this PR is about.
`RouterTestingHarness` can assert the actual navigation, and it is not much
longer:
```ts
@Component({ standalone: true, template: '' })
class DashboardStub {}
it('sends the bare feature path to the dashboard', async () => {
TestBed.configureTestingModule({
providers: [
provideRouter([
{ path: 'calendars', children: CALENDARS_ROUTES },
{ path: 'dashboard', component: DashboardStub },
]),
],
});
const harness = await RouterTestingHarness.create();
await harness.navigateByUrl('/calendars');
expect(TestBed.inject(Router).url).toBe('/dashboard');
});
```
Note the stub dashboard route — `redirectTo: '/dashboard'` is absolute, so
it resolves against the whole router config rather than this feature's, and a
test wiring up only `CALENDARS_ROUTES` would have nothing to land on.
The same shape also lets you assert the case that actually worries me, in
one more line: navigating to `/calendars/groups/1` and expecting the URL to
stay put.
Entirely your call — this is a suggestion, not a blocker. The change itself
is correct and I would be happy to see it merged as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
