Skip to content

Fix undefined sorting params in playlists table - #1689

Open
owi92 wants to merge 1 commit into
opencast:r/21.xfrom
owi92:fix-playlists-sort
Open

owi92 wants to merge 1 commit into
opencast:r/21.xfrom
owi92:fix-playlists-sort

Conversation

@owi92

@owi92 owi92 commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

There was a bug where missing values for sort parameters of new tables were overriding the new default values.
This fix is to change merging order, i.e. to make this work the other way around. This will only affect missing values, so actual stored values will still take precedence over the defaults.

Fixes #1688

There was a bug where missing values for sort parameters
of new tables where overriding the new default values.
This fix is to change merging order, i.e. to make this work the
other way around. This will only affect missing values, so
actual stored values will still take precedence over the defaults.
@owi92 owi92 added the type:bug Something isn't working label Oct 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org 

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

This pull request is deployed at test.admin-interface.opencast.org/1689/2026-10-15_08-13-21/ .
It might take a few minutes for it to become available.

@Arnei Arnei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works. The stuff below is less "request changes" and more "comprehension questions".

Comment thread src/store.ts
Comment thread src/utils/resourceUtils.ts Outdated
// by setting `undefined:undefined`
const sortBy = getTableSortingForResource(state, resource);
const direction = getTableDirectionForResource(state, resource);
if (sortBy && direction && direction !== "NONE") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is like a second safe guard beyond the stateReconciler, and not strictly necessary? Should there be some kind of warning logging statement, so that at least developers can be made aware that something is wrong?

Does the backend not take NONE as a value? (Sure, if we want to sort by none we might as well not sent any sort at all, this is really a technicality).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it's not strictly necessary and I debated whether to include this. I suppose we could add a logging statement, or just remove this again.
I am actually not sure if the backend takes "NONE", I just kept that condition from before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it to you whether you want to add a logging statement or remove this, I'm fine with either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ok lets just leave it as is.

@gregorydlogan

Copy link
Copy Markdown
Member

This should probably go into r/21.x now too

@owi92
owi92 changed the base branch from develop to r/21.x October 15, 2026 07:52
@owi92
owi92 force-pushed the fix-playlists-sort branch from ae5ad54 to 5faf60b Compare October 15, 2026 08:12

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default Playlists view is getting HTTP 400s

3 participants