Add admin page for site redirects #3502
Conversation
| "/site-redirects/new", | ||
| async (req: Request, res, trx) => { | ||
| const { source, target } = req.body | ||
| if (await redirectWithSourceExists(trx, source)) { |
There was a problem hiding this comment.
Should we maybe make source unique in the DB?
There was a problem hiding this comment.
Yeah that wouldn't hurt. Can you add a migration for it? For this we still use typeorm, the docs are in the /db folder in the readme.
rakyi
force-pushed
the
site-redirects-admin
branch
3 times, most recently
from
5310272 to
02bd8ba
Compare
db/model/Redirect.ts
Outdated
| `WITH RECURSIVE redirect_chain AS ( | ||
| -- Base case: Start with the target of the proposed new redirect. | ||
| SELECT source, target | ||
| FROM redirects | ||
| WHERE source = ? | ||
|
|
||
| UNION ALL | ||
|
|
||
| -- Recursive step: Follow the chain of redirects. | ||
| SELECT r.source, r.target | ||
| FROM redirects r | ||
| INNER JOIN redirect_chain rc ON rc.target = r.source | ||
| ) | ||
| -- Check if the new source appears in the redirect chain starting from | ||
| -- the new target. | ||
| SELECT COUNT(*) > 0 AS cycleExists | ||
| FROM redirect_chain | ||
| WHERE source = ? OR target = ?`, |
There was a problem hiding this comment.
I didn't word this carefully in the issue, but I think we want to be stricter which happens to also make this query easier: I think we want to avoid not only cycles but also any chains. If there is already a redirect for /B to /C, and we are about to add a redirect /A to /B then we should just fail and tell the user that there is already a redirect in place for /B to /C and they should just redirect from /A to /C directly.
Doing this then let's us do away with the recursive query (which is cool :) but a bit hard to grok) - instead we can just check if the source matches any existing target or the target matches any existing source.
There was a problem hiding this comment.
I reworked the validation.
| "/site-redirects/new", | ||
| async (req: Request, res, trx) => { | ||
| const { source, target } = req.body | ||
| if (await redirectWithSourceExists(trx, source)) { |
There was a problem hiding this comment.
Yeah that wouldn't hurt. Can you add a migration for it? For this we still use typeorm, the docs are in the /db folder in the readme.
| import { AdminAppContext } from "./AdminAppContext.js" | ||
| import { AdminLayout } from "./AdminLayout.js" | ||
|
|
||
| const VALID_PATTERN = /^\/$|^.*[^\/]+$/ |
There was a problem hiding this comment.
I would be stricter here and have a different pattern for source and target. Sources have to start with a slash. Targets have to start either with a slash or http://.
I think it is also nice to document regexps a bit more since they are so dense - I tried one form of this e.g. here
There was a problem hiding this comment.
Changed. I made the validation messages more specific, which should also serve as docs for the regexps. Let me know if you think it's not sufficient.
| <main> | ||
| <form onSubmit={handleSubmit(onSubmit)}> | ||
| <div className="form-row"> | ||
| <div className="form-group col-12 col-md-4"> |
There was a problem hiding this comment.
I think we should explain to the admin users a bit what is expected of them here. Yes, our admin UIs are very barebones but our authors are not very versed in the technical details of HTTP redirects so we should explain a bit what is happening here.
I think it is important to tell them that source has to start with a slash and should not contain query params or fragments (you may also want to strip those if they are given). The target can contain full urls or relative urls starting with a slash and here both query params and fragments are allowed. Maybe give an example for query param and fragment in parens as it might not be obvious what these things are.
There was a problem hiding this comment.
It is also worth calling out that to redirect to our homepage they should use a single slash as the target
There was a problem hiding this comment.
Added the instructions.
| {...register("source", { | ||
| required: "Please provide a source.", | ||
| pattern: { | ||
| value: VALID_PATTERN, |
There was a problem hiding this comment.
In addition to checking for a leading slash I think we should also either disallow query params/fragments via the regex or strip them out before sending the redirect to the API.
There was a problem hiding this comment.
They are now stripped on submit.
rakyi
force-pushed
the
site-redirects-admin
branch
2 times, most recently
from
50043d2 to
24ea7ac
Compare
rakyi
force-pushed
the
site-redirects-admin
branch
from
24ea7ac to
762ab77
Compare
|
@danyx23 do you have an idea why the refresh-etl job would be failing? Rebasing on master didn't help. |
There was a problem hiding this comment.
Very nice, thanks for the changes! I had some ideas for slight tweaks and thought it's better to just add them in a commit of my own. Feel free to tweak my styling ideas, I just felt that using concrete examples form our site works better than generic a=b examples. If you are happy with then please ship it! :)
|
@rakyi No I don't know what is causing the ETL update to fail but it's not relevant for commits in this repo (it is only used when the grapher infrastructure is used as part of PRs in the ETL repository). I think it's good to ship! |
I got heavily inspired by
RedirectsIndexPage.tsx. I usedreact-hook-formsince I had good experience with it before, and it makes the form handling nice.Server validation error leads to a generic error popup on the frontend, which is bad UX, but it seems this is what we do also elsewhere. Let me know if there is a good way to avoid that.