Restore (some) shareable links #981
Conversation
Due to the previous commit, the current modal page is no longer local state that is cleaned up when the series details modal is unmounted.
This necessitates moving the actual rendering of said modal from the individual action cells into the main tables, so that we don't render multiple instances.
This makes it to be more in line with the corresponding event details modal.
JulianKniephoff
force-pushed
the
shareable-links
branch
from
66c6411 to
b12d2d5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I hope the example isn't too bad. In the past, I had trouble with unescaped URLs in SSO, Studio, etc. redirect scenarios. |
|
That was quick 👀 I wanted to provide an example from the test deployment but that 404s on me. 🤔 Anyway, no, it's pretty bad alright. 👌😂 |
|
:D Well, any URL should work if properly escaped. So people need to make sure, but that was already the case for the old UI. |
|
If this bothers us we can still tweak it a little bit in two ways mainly:
A completely different approach would preserve editability as well: We could just put all of that stuff in the application router and have nice urls like |
This comment was marked as resolved.
This comment was marked as resolved.
This lead to the app not remembering what page (events vs. series) the user was on.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
In addition to the conflicts and a little bug I just pushed a fix for, I just noticed that metadata for a series doesn't load when you visit a shared link directly. Converting to draft for now, but will look into it shortly. |
This makes the series details modal work when coming from a "direct link." It also makes it work more like the events modal.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Use Run test server using develop.opencast.org as backend: Specify a different backend like stable.opencast.org: It may take a few seconds for the interface to spin up. |
|
This pull request is deployed at test.admin-interface.opencast.org/981/2024-11-25_15-58-42/ . |
Everything should now be fixed! |
There was a problem hiding this comment.
More of a thought dump than a review, but here you are.
Long URLs suck. Sure, users can throw an URL shortener at it, but it would be better if they did not have to do that in the first place.
URL now leaks data, e.g. metadata. As you suggested, applying transformations sounds like a reasonable way to solve that.
Having to lift modal state (state that only interests the modal component) into redux seems bad practice to me. You mentioned another approach would be to use the application-router. What is the issue with that compared with the current redux-persist approach?
Code looks clean, me like. Can also confirm that the whole approach generally works.
There are some parts of the event modal that will not load their information. For example assets subtabs or workflow operation details.
Do you know if, with your current approach, it would be possible to persist a state both in store and in url? For example, if I wanted to change the code persist something about event details in local storage, could I do that?
While this is a matter of taste, it's not absurd to have all application state in Redux. In fact, if you want to reap the full benefits, you kind of have to. Mixing Redux state and local state can make stuff like undo, time travel debugging, etc. weird. Not that we use any of these rn, but still. That's not the code smell here, I think. The fact that that state is then also reflected in the URL complicates things, which is something that we might not want. We should certainly look further into which parts of the state are exposed like that, if we do want it. So yeah, before we apply transformations (which we might still want because long URLs suck) I would try to reduce the state we expose.
Not really an issue, but one theoretical advantage of this is that we have one central place where we define what parts of the state are in the URL, by just annotating the appropriate bits of state. In theory, assuming the relevant state is in Redux already, which I hoped more of it would be. Anyway, in the router based approach, we need to change all the relevant components to accept appropriate props from the router so that it can tell them to display the modal, etc., or alternatively they need to consult the router themselves. These would be less general/global and more ad-hoc, case-by-case changes, I feel like.
Yes, but it's not trivial. You would have to extend the storage mechanism I wrote for this (or wrap it and the localstorage one in another one, is probably what I would do), and then think a bit about what the source of truth is supposed to be.
Good catch, I'll look into it, but we should probably decide whether or not we want to move forward with this approach. 🤔 I will probably bring this up again in tomorrow's technical. |
JulianKniephoff
added
the
discuss in dev meeting
|
This pull request has conflicts ☹ |
Addresses #69, but see also my remarks in #69 (comment); I'm not sure if it closes it because people might want more state in their share links.
TL;DR this puts some of the Redux state in the URL so that you can share "deep links" into the application, as in: provide people with a link to the admin UI that directly opens the event details for a given event.
Right now you can link to:
The latter specifically necessitated putting some more state into Redux in the first place. Plus some adjacent refactoring. 🤷♀️
Creates links like http://localhost:3000/?persist%3AseriesDetails=%7B%22modal%22%3A%22%7B%5C%22show%5C%22%3Afalse%2C%5C%22page%5C%22%3A0%2C%5C%22series%5C%22%3A%7B%5C%22id%5C%22%3A%5C%22ID-av-portal%5C%22%2C%5C%22title%5C%22%3A%5C%22AV-Portal+Content%5C%22%7D%7D%22%2C%22_persist%22%3A%22%7B%5C%22version%5C%22%3A-1%2C%5C%22rehydrated%5C%22%3Atrue%7D%22%7D&persist%3AeventDetails=%7B%22modal%22%3A%22%7B%5C%22show%5C%22%3Atrue%2C%5C%22page%5C%22%3A0%2C%5C%22event%5C%22%3A%7B%5C%22end_date%5C%22%3A%5C%222024-11-14T01%3A13%3A19Z%5C%22%2C%5C%22agent_id%5C%22%3A%5C%22%5C%22%2C%5C%22displayable_status%5C%22%3A%5C%22EVENTS.EVENTS.STATUS.PROCESSED%5C%22%2C%5C%22needs_cutting%5C%22%3Afalse%2C%5C%22source%5C%22%3A%5C%22ARCHIVE%5C%22%2C%5C%22title%5C%22%3A%5C%22Weitsprung%5C%22%2C%5C%22has_open_comments%5C%22%3Afalse%2C%5C%22has_preview%5C%22%3Atrue%2C%5C%22technical_presenters%5C%22%3A%5B%5D%2C%5C%22has_comments%5C%22%3Afalse%2C%5C%22technical_end%5C%22%3A%5C%222024-11-14T01%3A13%3A19Z%5C%22%2C%5C%22series%5C%22%3A%7B%5C%22id%5C%22%3A%5C%22ID-av-portal%5C%22%2C%5C%22title%5C%22%3A%5C%22AV-Portal+Content%5C%22%7D%2C%5C%22presenters%5C%22%3A%5B%5C%22W%C3%A4lken%2C+Paul%5C%22%5D%2C%5C%22technical_start%5C%22%3A%5C%222024-11-14T01%3A03%3A03Z%5C%22%2C%5C%22location%5C%22%3A%5C%22%5C%22%2C%5C%22managedAcl%5C%22%3A%5C%22public%5C%22%2C%5C%22workflow_state%5C%22%3A%5C%22SUCCEEDED%5C%22%2C%5C%22id%5C%22%3A%5C%22ID-weitsprung%5C%22%2C%5C%22start_date%5C%22%3A%5C%222024-11-14T01%3A03%3A00Z%5C%22%2C%5C%22event_status%5C%22%3A%5C%22EVENTS.EVENTS.STATUS.PROCESSED%5C%22%2C%5C%22publications%5C%22%3A%5B%7B%5C%22name%5C%22%3A%5C%22EVENTS.EVENTS.DETAILS.PUBLICATIONS.ENGAGE%5C%22%2C%5C%22id%5C%22%3A%5C%22engage-player%5C%22%2C%5C%22url%5C%22%3A%5C%22https%3A%2F%2Fdevelop.opencast.org%2Fplay%2FID-weitsprung%5C%22%2C%5C%22enabled%5C%22%3Atrue%2C%5C%22hiding%5C%22%3Afalse%7D%5D%2C%5C%22date%5C%22%3A%5C%222024-11-14T01%3A03%3A00Z%5C%22%2C%5C%22selected%5C%22%3Afalse%7D%2C%5C%22workflowTabHierarchy%5C%22%3A%5C%22entry%5C%22%2C%5C%22assetsTabHierarchy%5C%22%3A%5C%22entry%5C%22%2C%5C%22workflowId%5C%22%3A%5C%22%5C%22%7D%22%2C%22_persist%22%3A%22%7B%5C%22version%5C%22%3A-1%2C%5C%22rehydrated%5C%22%3Atrue%7D%22%7D#, which is not pretty, but oh well, no one is expected to read and/or write these manually.