Redirect to a different site if deleting idSite in URL #12696
Conversation
diosmosis
added
not-in-changelog
diosmosis
force-pushed
the
12617-redirect-after-delete-site
branch
from
5aa4827
to
f700260
Compare
| // if the current idSite in the URL is the site we're deleting, then we have to make to change it. otherwise, | ||
| // if a user goes to another page, the invalid idSite may cause a fatal error. | ||
| if (broadcast.getValueFromUrl('idSite') === $scope.site.idsite) { | ||
| var site = $scope.adminSites.sites.find(function (site) { return site.idsite !== $scope.site.idsite }); |
There was a problem hiding this comment.
I think .find might not be available eg in IE10
There was a problem hiding this comment.
👍 will fix
Would be great to use something like babel-polyfill in the future :)
…to a different site to avoid later errors.
diosmosis
force-pushed
the
12617-redirect-after-delete-site
branch
from
f700260
to
916525f
Compare
|
Updated |
|
|
||
| // if the current idSite in the URL is the site we're deleting, then we have to make to change it. otherwise, | ||
| // if a user goes to another page, the invalid idSite may cause a fatal error. | ||
| if (broadcast.getValueFromUrl('idSite') === $scope.site.idsite) { |
There was a problem hiding this comment.
You need a lose check using == instead of ===. I tested it with Mysqli adapter and didn't work because with Mysqli it is an integer and not a string. Using == works.
There was a problem hiding this comment.
Argh, that's annoying. I think I'll convert the idsite to string, since it's considered bad practice to use == in JS (which I'm sure you're aware :) ). (Unless you have an objection.)
There was a problem hiding this comment.
Where would you cast it to string? It's fine by me but would personally just use ==. Of course it is a bad practice but it doesn't mean it should never be used or something just for the sake of it. In this case, we know idSite is eg never 0 or something and it doesn't really cause any difference to use == vs ===. Might be easier to implement and read in this case. Up to you though.
There was a problem hiding this comment.
Yes, but pretty much every linter will complain about that and some transpilers like coffeescript will convert == to === implicitly. It's really not accepted by the community (not that I'm saying it shouldn't be accepted or anything, it's the opposite in the PHP community), but using it in JS is kind of like adding technical debt, since it will be something that we'd have to change when moving to more modern toolchains. Different languages, different standards.
What I was going to do is just cast $scope.site.idsite to string, ie var currentSite = '' + $scope.site.idsite; w/ a comment about mysqli.
There was a problem hiding this comment.
We're not using coffeescript and neither transpile anything etc ;) but totally fine to add the casting as long as it is always updated etc
There was a problem hiding this comment.
Not yet, but if matomo ever decides to move to Angular 2, we're going to have to start. But I'll just use ==. If it inspires this much discussion, then I'd rather be on the same page as my teammates then do something that isn't considered a matomo standard :)
There was a problem hiding this comment.
As said it's all fine to cast ;) When/if we move to angular we will have to fix things everywhere anyway and already cast the output in the API to work with consistent types independent of DB adapter. If it's only a one liner and doesn't break anything else it should be all good to cast.
|
|
||
| var otherSite; | ||
| for (var i = 0; i !== sites.length; ++i) { | ||
| if (sites[i].idsite !== $scope.site.idsite) { |
|
I think it might make still sense to swallow this exception, or not really sense but might cause many other random bugs we cannot foresee so better to maybe just keep it now 👍 |
…O returns strings.
|
Updated. |
Avoids later errors if user clicks on a link w/ the invalid idSite.
Fixes #12617
@matomo-org/core-team I wonder if it would also be good not to swallow this exception: https://github.com/matomo-org/matomo/blob/3.x-dev/core/Plugin/Controller.php#L133
The error would have made a lot more sense to me w/ an "invalid idSite" message.
TODO: