Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #36, #997 respect workflow displayOrder, add filter workflows #1101

Closed
wants to merge 2 commits into from

Conversation

JamesUoM
Copy link
Contributor

@JamesUoM JamesUoM commented Jul 6, 2023

Respect workflow displayOrder
Add filter workflows by displayOrder (min/max config settings)

UoM: MAT-477, MAT-478

@JamesUoM JamesUoM added type:enhancement New feature or request type:usability Usability improvements labels Jul 6, 2023
@JamesUoM
Copy link
Contributor Author

JamesUoM commented Jul 6, 2023

deploy.opencast.org only has one workflow. To help with testing you can inject a dummy workflow by editing
src/main/WorkflowSelection.tsx

    const filterWorkflows = (workflowFilter: IWorkflowConfiguration | undefined, workflows: Workflow[]) => {
    // DEBUG
    let w : Workflow = {
      id: "1235",
      name: "test workflow",
      description: "bobbins",
      displayOrder: 600
    }

    workflows = [workflows[0], w]
    ...

…lter workflows by displayOrder

UoM: MAT-477, MAT-478
@JamesUoM JamesUoM force-pushed the f/filter-workflows branch from 0c84ff0 to ce95d99 Compare July 6, 2023 11:19
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

This pull request is deployed at test.editor.opencast.org/1101/2023-07-06_11-20-09/ .
It might take a few minutes for it to become available.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Could you add the new settings to the public/editor-settings.toml as well?

I can crash the editor with the following setting

min = 1001
max = 1000

Results in this error:

Uncaught TypeError: Cannot assign to read only property '0' of object '[object Array]'
    at Array.sort (<anonymous>)
    at WorkflowSelection (WorkflowSelection.tsx:58)

I have four workflow configured, with displayOrder values 1010, 1000, 100 and no value.

@Arnei
Copy link
Member

Arnei commented Jul 7, 2023

Small remark: The editor has been respecting displayOrder, but apparently been sorting the wrong way around:

state.workflows = action.payload.workflows.sort((n1: { displayOrder: number; }, n2: { displayOrder: number; }) => {
. I suppose that code can go then.

src/config.ts Show resolved Hide resolved
@JamesUoM
Copy link
Contributor Author

I have four workflow configured, with displayOrder values 1010, 1000, 100 and no value.

It's not the min>max but something weird triggered by the sort() when a displayOrder is undefined. You can't sort "in place" and need to create a copy.

min>max results in zero workflows selected and so the warning is issued and all workflows are returned.

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/1101/2023-07-10_14-53-55/ .
It might take a few minutes for it to become available.

@JamesUoM JamesUoM requested a review from Arnei July 28, 2023 08:17
Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

No complaints about respecting display order, but I'm really not sure about the filtering.

Comment on lines +171 to +176
# Filter the publishing workflows by their displayOrder value
# min <= displayOrder <= max
# Type: number
# Default: undefined (unbound)
#min =
#max =
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this really makes sense and would like to hear your reasoning behind this. I find this extremely unintuitive. Why should the order determine if a workflow is being shown or not? As an admin, I could see it easily happen that I want to change the order and this causes the workflow to accidentally disappear.

More than that, with the old editor being sunsetted and about to be removen, the new editor is the only place in Opencast where the tag editor is being used. This means that you can just tag the workflows and only the workflows you want to be shown as editor.

This is much more straightforward ans also means that the same functionality is not being duplicated (you are essentially trying to add a second configuration option which does the same thing as an existing one).

That means, unless you have a good argument for adding this, I really think we shouldn't add this.

@JamesUoM
Copy link
Contributor Author

JamesUoM commented Aug 14, 2023 via email

@github-actions
Copy link

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

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Sep 13, 2023
@KatrinIhler
Copy link
Member

@JamesUoM I agree with Lars here, I don't think the filtering is something we want in the upstream. But the rest of the changes seem useful, could you maybe update this PR or file those separately?

@JamesUoM
Copy link
Contributor Author

JamesUoM commented Oct 4, 2023

To keep it clean and tidy I've created a replacement PR #1179

@JamesUoM JamesUoM closed this Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:conflicts Conflicts with another pull request or issue type:enhancement New feature or request type:usability Usability improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants