Add active/paused toggle #535
Conversation
| }: PaMessageRowProps) => { | ||
| const navigate = useNavigate(); | ||
| const start = new Date(paMessage.start_datetime); | ||
| const end = | ||
| paMessage.end_datetime === null ? null : new Date(paMessage.end_datetime); | ||
|
|
||
| const [paused, setPaused] = useState(!!paMessage.paused); |
There was a problem hiding this comment.
Rather than adding more state here, we could pass a callback that allows updating the underlying PA message state directly, and then just render that.
| }: PaMessageRowProps) => { | ||
| const navigate = useNavigate(); | ||
| const start = new Date(paMessage.start_datetime); | ||
| const end = | ||
| paMessage.end_datetime === null ? null : new Date(paMessage.end_datetime); | ||
|
|
||
| const [paused, setPaused] = useState(!!paMessage.paused); | ||
|
|
||
| const stopPropagation = (event: React.MouseEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
This variable could have a better name, e.g. togglePaused
|
|
||
| const stopPropagation = (event: React.MouseEvent<HTMLDivElement>) => { | ||
| event.stopPropagation(); | ||
| updateExistingPaMessage(paMessage.id, { |
There was a problem hiding this comment.
Without any error handling on this fetch, this could silently fail and leave the system in an unexpected state. We should at least be catching exceptions and checking that the result is ok, even if all we do is throw an error toast to let them know that something went wrong.
I'd lean slightly in the other direction, given the concept of a message being "paused" is specific to Screenplay. I don't think RTS should have to also understand that — it should just get a list of messages it should play. If it also has to filter out paused messages, the "footprint" of this feature becomes larger and it could be harder to maintain.
I didn't realize the endpoint used by the Screenplay frontend and the endpoint used by RTS were actually the same. I think this is the solution I'd favor. We probably want to be able to do whatever we like with our "internal API" to the frontend, without affecting our external "contract" with RTS. |
|
@digitalcora Actually I was sort of mistaken - they do use different endpoints, but they share the same query. If we just add a new query or some filtering that accounts for the paused state, that should be easy enough. |
| @@ -141,7 +159,7 @@ const PaMessagesPage: ComponentType = () => { | |||
| className={cx("button", { active: stateFilter === "active" })} | |||
| onClick={() => setStateFilter("active")} | |||
| > | |||
| Active | |||
| Now | |||
There was a problem hiding this comment.
Something I'm thinking about while working on the End Now task. We should also consider changing all references of active instead of just the label. This is a little more involved than normal because these values are passed to the server to help form the queries needed to filter the data. I still think it would be valuable to eliminate the inconsistent naming.
There was a problem hiding this comment.
I thought about that too, but now just seems a little awkward to me as an adjective. eg get_now() is a little weird compared to get_active. At the same time, it would be helpful to have two distinct words for describing messages that are within their active period and unpaused versus just generally within their active period. I sort of wish we used current rather than now or something.
There was a problem hiding this comment.
That's a good point. Maybe this is something to bring up to Mary? I didn't think about how it's kind of the outlier here not being an adjective (or at least an awkward one like you said). I agree with her that we can't use Active anymore, but your suggestion of Current sounds better to me.
There was a problem hiding this comment.
No reason we couldn't use current as our internal name and "Now" as the user-facing name.
There was a problem hiding this comment.
@digitalcora that works for me. I just pushed a change to use current internally.
| paused: !paMessage.paused, | ||
| } as UpdatePaMessageBody); | ||
|
|
||
| if (result.status === 200) { |
There was a problem hiding this comment.
This is better, but it still doesn't catch network errors that cause the request to fail entirely. The way I usually like to handle this is to modify the API function (updateExistingPaMessage in this case) to include something like:
// using ok allows any successful response, e.g. 201
if (!response.ok) {
const error = new Error("request failed")
// attach any available error information from the server
throw error
}Now the API function's promise will resolve on success and reject on any failure, so you can use it like:
try {
const payload = await updateExistingPaMessage(...)
// do happy path stuff
}
catch (error) {
// display error message
}There was a problem hiding this comment.
Thinking of the other usage of this API function on the EditPaMessage page if we modified the function this way, how would we attach the list of errors to the returned Error object when the server returns that? It doesn't seem like Error has a property that lets us easily pass along the response body.
There was a problem hiding this comment.
There are two approaches, and it kind of depends on how sophisticated the error reporting needs to be. If we just want to show a little diagnostic message, it might be enough to just read/parse the body and stick it in as the message string on the Error, and then just display that. If we need something fancier, then we could attach arbitrary data to the Error object by just assigning a custom property. The latter probably isn't needed, but I just wanted to mention it.
There was a problem hiding this comment.
For the edit page, the current logic takes the object keys from the body and passes that array to the toast which renders a list of the errors the user needs to address. I tried an approach where we just throw the array itself as the error which seems okay to me. Let me know what your thoughts are though
There was a problem hiding this comment.
This seems good for now. If we like this pattern and want to apply it more generally in the future, we can formalize things by e.g. adding custom error subtypes with known data shapes, but that's probably premature at this point.
| where: | ||
| ^current_service_day_of_week in m.days_of_week and | ||
| m.start_datetime <= ^now and | ||
| ((is_nil(m.end_datetime) and m.alert_id in ^alert_ids) or m.end_datetime >= ^now) and |
There was a problem hiding this comment.
A big chunk of this clause looks the same as the one in current. Can we use composition somehow to reuse that?
There was a problem hiding this comment.
ah yes, good point!
Asana task: Pause/Active toggle
Description
This PR adds a toggle to the PA Messages table that allows a user to pause or unpause a PA Message. The toggle updates the pause tag in the DB.
Additionally, if a PA Message is updated in a way where it is going to be either in or out of the active period of said PA Message, we will set the paused state to unpaused.
When a PA Message is set to paused, it should not trigger readouts. Note that this PR doesn't update the query that returns active PA Messages, which is also used by RTS when querying for active messages. I have the opinion that it would be simpler to do the filtering on the RTS client-side since if we filter out paused messages on the server, they won't be returned to Screenplay and therefore we won't show them in the PA Messages table anymore. We'd have to split it out into a different endpoint or add a parameter for including paused messages or something. I'm open to other thoughts though.