Show registration modal only when unregistered#1600
Conversation
|
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/1600/2026-05-11_21-10-31/ . |
| if (registration == null) { | ||
| dispatch(fetchRegistration()); | ||
| } |
There was a problem hiding this comment.
This will dispatch a redux action (and thus a fetch request) every time the component rerenders and registration is still null. This could potentially cause multiple fetch requests. To avoid this, we usually use a useEffect that only runs once on component mount for fetch requests. (see the useEffect in e.g. SeriesDetails.tsx).
If you need to monitor the state of the action, you would usually add a variable to the redux state that tracks the action state (LOADING, SUCCEEDED, FAILED what have you). Then track the action state in your component via a selector and use a useEffect to respond properly.
And if you don't actually need all that redux boilerplate, you could also do the fetch requesting and state tracking here with useEffect and useState.
…in suboptimal cases
75e3a11 to
298d1a8
Compare
|
Now that the GHA issues are done, I made a final change and I think this is ready for re-review @Arnei |
| .wide-text{ | ||
| padding-right: 5px; | ||
| } |
There was a problem hiding this comment.
| .wide-text{ | |
| padding-right: 5px; | |
| } | |
| .wide-text{ | |
| padding-right: 5px; | |
| } |
There was a problem hiding this comment.
You're using 2 spaces, but the file uses 4.
| useEffect(() => { | ||
| if (!user) { return; } | ||
|
|
||
| const isAdmin = user.isAdmin || user.isOrgAdmin; | ||
| const isLocalhost = window.location.hostname === "localhost"; | ||
| const lastDismissed = localStorage.getItem("adopterModalDismissed"); | ||
| const THIRTY_DAYS = 30 * 24 * 60 * 60 * 1000; | ||
| const dismissedLongEnough = !lastDismissed || Date.now() - parseInt(lastDismissed) > THIRTY_DAYS; | ||
|
|
||
| if (isAdmin && !isLocalhost && dismissedLongEnough) { | ||
| showRegistrationModal(); | ||
| } | ||
| }, [user]); | ||
| dispatch(fetchRegistration()); | ||
| dispatch(fetchLatestToU()); | ||
| dispatch(fetchIsUpToDate()); | ||
|
|
||
| if (!user) { return; } | ||
|
|
||
| const isAdmin = user.isAdmin || user.isOrgAdmin; | ||
| const isLocalhost = window.location.hostname === "localhost"; | ||
| const lastDismissed = localStorage.getItem("adopterModalDismissed"); | ||
| const THIRTY_DAYS = 30 * 24 * 60 * 60 * 1000; | ||
| const dismissedLongEnough = !lastDismissed || Date.now() - parseInt(lastDismissed) > THIRTY_DAYS; | ||
|
|
||
| if (isAdmin && !isLocalhost && dismissedLongEnough && registration == null) { | ||
| showRegistrationModal(); | ||
| } | ||
| }, [user, registration, dispatch]); |
There was a problem hiding this comment.
This can cause an infinite loop. dispatch(fetchRegistration()); will eventually update registration, which triggers a rerun of the useEffect hook, which causes dispatch(fetchRegistration()); to be run again...
I recommend putting the dispatch(fetch....'s into their own useEffect hook that runs only once on mount. If you need to run them again, you should probably do so conditionally?
| )} | ||
| </li> | ||
| ))} | ||
| {!registering && |
| onClick={() => showAdoptersRegistrationModal()} | ||
| > | ||
| <span className="wide-text">Registration</span> | ||
| <span className="multi-value multi-value-yellow">Unregistered</span> |
There was a problem hiding this comment.
multi-value-yellow does not seem to exist (maybe I removed it in my cleaning frenzy?).
| <ButtonLikeAnchor | ||
| onClick={() => showAdoptersRegistrationModal()} | ||
| > | ||
| <span className="wide-text">Registration</span> |
| @@ -0,0 +1,110 @@ | |||
| import { PayloadAction, createSlice } from "@reduxjs/toolkit"; | |||
There was a problem hiding this comment.
Inconsistent use of spaces and tabs in this file.
| // These are used for thunks | ||
| extraReducers: builder => { | ||
| builder | ||
| /* .addCase(fetchRegistration.pending, state => { |
| error: boolean | ||
| }; | ||
|
|
||
| type Temp = { |
| export type RegistrationState = { | ||
| registration: Registration | null, | ||
| latestToU: string, | ||
| isRegistering: boolean, |
There was a problem hiding this comment.
I feel like isRegistering is a bit confusingly named? This is not about being in the process of registering, is it? Maybe canRegister would be clearer?
| name: "registration", | ||
| initialState, | ||
| reducers: { | ||
| setError(state, action: PayloadAction<{ |
There was a problem hiding this comment.
I don't see setError nor error being used anywhere. Leftover code?
This PR only shows the registraiton modal when the adopter has not already registered.
This PR contains a few extra bits that haven't been turned on yet - notably the warning bell stuff for when the core can't reach the registration server.This PR now uses the bell if the user is not registered. Notably:


If you haven't registered at all:
If you have registered, but haven't agreed to the latest ToU:
Clicking on either of these notices opens the registration modal.
TODOs:
Fixed #1516