Display critical error if Opencast does not provide a video duration

[Arnei]

The duration return by the backend can be "null". This is not handled by the frontend at all, resulting in an erroneous state.

Now, the critical error page is shown to inform the user that editing is impossible.

Resolves #253 and provides an alternative to #255, as determining the duration in the frontend does not seem to work properly.

[github-actions]

This pull request is deployed at test.editor.opencast.org/329/2021-05-31_10-44-08/ .
It might take a few minutes for it to become available.

[lkiesow]

A critical error for a missing duration seems excessive. What exactly do we need this duration for so desperately?

[Arnei]

The cutting view relies on the duration to determine where the user is setting cut marks in the cutting view, since the backend asks for absolute timestamps.

It may be fine to just display an error in the cutting view, since this should not effect other views like metadata.

[github-actions]

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

[github-actions]

This pull request is deployed at test.editor.opencast.org/329/2022-04-21_11-34-24/ .
It might take a few minutes for it to become available.

[github-actions]

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

[github-actions]

This pull request is deployed at test.editor.opencast.org/329/2022-07-06_09-42-30/ .
It might take a few minutes for it to become available.

[mtneug]

@Arnei did you find out why Opencast does not report durations at times?

[Arnei]

@Arnei did you find out why Opencast does not report durations at times?

I'll have to admit that I do not remember if we ever found out or not.

[LukasKalbertodt]

I have not yet found a way to test this (to reproduce the null duration), but the code is straight forward and I'm pretty sure it does what the PR says. I only found one thing that could be improved.

[LukasKalbertodt]

My comments are resolved!

Unfortunately, there is still a type error (see CI)

[Arnei]

Maybe that is good indicator for why we shouldn't allow duration to be of type null. The editor expects there to be a duration. it is not equipped to work without a duration. So it needs to be provided a duration. if it isn't, it breaks (and now hopefully displays an error message). It does not make much sense to me to add | null to every parameter passed by Opencast and add a huge amount of null checks to the code if the code isn't supposed to work with a null value in the first place.

[github-actions]

This pull request is deployed at test.editor.opencast.org/329/2023-06-12_14-10-05/ .
It might take a few minutes for it to become available.

[LukasKalbertodt]

Maybe that is good indicator for why we shouldn't allow duration to be of type null. The editor expects there to be a duration. it is not equipped to work without a duration. So it needs to be provided a duration. if it isn't, it breaks (and now hopefully displays an error message). It does not make much sense to me to add | null to every parameter passed by Opencast and add a huge amount of null checks to the code if the code isn't supposed to work with a null value in the first place.

Fair, and I strongly agree with not putting null checks everywhere. But I think the fault lies in the structure here. I think the error should not be "thrown" there but rather in line 200 (state.duration = action.payload.duration). Here, the right hand side is any. Here, it should be checked whether the value is actually a non-null number. If so, assign it to state.duration (which has the type number without null). If not, throw an error. But already letting a null value INTO state.duration and only catching it later in a component seems suboptimal to me. Also, who says that that Video component is the first to receive that null duration? Maybe in the future, it just so happens that another component receives it and already breaks.
I have too little knowledge of the redux structure and stuff, but maybe actually throwing an error in that line and using error boundaries is a better solution?

To be clear: I don't want to waltz in here, into a project I haven't written code for, and tell yall how to run your business :D Just some information and opinions. Do with that what you will ^_^

[Arnei]

Do agree that the error should be thrown asap in the line you suggested. Unfortunately that's non-trivial (or at least way more difficult than throwing the error Video.tsx, as direct inter-slice communication isn't really intended in redux), so I didn't do that.

[github-actions]

This pull request is deployed at test.editor.opencast.org/329/2023-06-12_15-03-47/ .
It might take a few minutes for it to become available.

[github-actions]

This pull request is deployed at test.editor.opencast.org/329/2023-06-12_15-33-36/ .
It might take a few minutes for it to become available.