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

Add additional features for Tobira integration #878

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

owi92
Copy link
Contributor

@owi92 owi92 commented Aug 12, 2024

This adds most of the additional requirements in #311.
Needs elan-ev/tobira#1225 and opencast/opencast#6091 to work correctly.
Only the last 7 10 commits need to be reviewed here, as the rest is based on #313.

Key changes/features:
(Edit: see updated screenshots in my comment further below)

  • The event details modal now has a Tobira tab which provides a direct link and list any pages in Tobira that feature the event:
    Bildschirmfoto 2024-08-12 um 15 02 32

  • The Tobira tab of the series details modal got the addition of an "edit path" button that lets admins edit paths of series that are already mounted in Tobira (with the condition that the page where the series is mounted does not contain any other blocks)
    Bildschirmfoto 2024-08-12 um 15 05 38Bildschirmfoto 2024-08-12 um 15 38 48

  • Selectable pages for adding and moving (i.e. updating the path of) series now feature an additional checkbox that shows which page is currently selected (and selectable at all)
    Bildschirmfoto 2024-08-12 um 15 07 09

Notable omissions/deviations:

  • @oas777 suggested to also add editing host-page paths of events. While technically possible, we don't allow choosing a host-page for events via the admin UI in the first place, so that would need to be added first. This needs some discussing and is currently out of scope for this PR.
  • @dagraf suggested adding a green checkmark when creating new subpages for hosting a series. Clicking that would then confirm the creation and disable editing the title and path. I decided against that since I already added the checkbox as per David's other suggestion, which serves a similar purpose. I feel like adding yet another checkmark to that is unnecessary and may wrongly suggest that the path will already be created when that is clicked.

If these omissions are dealbreakers, we should discuss them in an upcoming meeting.

Note: This PR is based on #313 which adds the Tobira integration. That should definitely be merged prior to this one! (it's only been open for like 4 months..)
I'll mark this as a draft until #313 is merged.
Closes #311

Copy link
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 ghcr.io/opencast/opencast-admin-interface:pr-878

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 ghcr.io/opencast/opencast-admin-interface:pr-878

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.

Copy link
Contributor

github-actions bot commented Aug 12, 2024

This pull request is deployed at test.admin-interface.opencast.org/878/2024-10-29_16-08-10/ .
It might take a few minutes for it to become available.

@owi92
Copy link
Contributor Author

owi92 commented Aug 13, 2024

I tried to make the Tobira tabs a little less "matryoschka"-like, as suggested in todays technical Meeting. This does however mean that these tabs won't follow the design examples set by other tabs.

They are still built like russian dolls (I didn't come up with that analogy but it's fitting).
Though I think that should be fixed in another PR.

Some updated screenshots:
Bildschirmfoto 2024-08-13 um 19 18 57
Bildschirmfoto 2024-08-13 um 19 19 20
Bildschirmfoto 2024-08-13 um 19 19 54

@owi92 owi92 force-pushed the tobira-things branch 5 times, most recently from 3838172 to 9f543db Compare August 15, 2024 09:54
Copy link
Contributor

github-actions bot commented Sep 2, 2024

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

@Arnei Arnei mentioned this pull request Sep 5, 2024
LukasKalbertodt added a commit to elan-ev/tobira that referenced this pull request Oct 8, 2024
)

This is necessary for an upcoming admin UI feature which will allow
admins to change the path of series pages with no other blocks (part of
opencast/opencast-admin-interface#311).

This shouldn't break any existing behaviour.
Related Opencast and admin UI PRs:
opencast/opencast#6091,
opencast/opencast-admin-interface#878.
Copy link
Contributor

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

KatrinIhler added a commit to opencast/opencast that referenced this pull request Oct 29, 2024
This adds
 - a) an endpoint to get paths of pages in Tobira that host an event
 - b) an endpoint to update the path of a series in Tobira

This shouldn't break any existing behaviour.

These are needed for additional requirements of
opencast/opencast-admin-interface#311.
Corresponding Tobira and admin-UI PRs:
elan-ev/tobira#1225,
opencast/opencast-admin-interface#878.

### Your pull request should…

* [ ] have a concise title
* [ ] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [ ] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [ ] include migration scripts and documentation, if appropriate
* [ ] pass automated tests
* [ ] have a clean commit history
* [ ] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
@owi92 owi92 marked this pull request as ready for review October 29, 2024 12:15
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.

Found a page crasher:

  • Go to the tobira tab in either the series details or when creating a new series.
  • Click on the "+ add subpage" button.
  • Try to switch to a different page (e.g. from a subpage to the homepage)
  • Observe blank page and "Uncaught Error: Maximum update depth exceeded." in the browser console.

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.

Bildschirmfoto vom 2024-10-29 15-04-49

This is an event without a series, so "Mount series" does not make sense here? Should it say "Mount event"? Also when clicking on the "Mount series" button and trying to mount, the mount post request seems to fail.

@owi92
Copy link
Contributor Author

owi92 commented Oct 29, 2024

Found a page crasher (...)

Hm I can't reproduce that. Is this happening every time you try that, even with a freshly started Admin UI, or are there some actions done before that might possibly provoke this?

This is an event without a series, so "Mount series" does not make sense here? Should it say "Mount event"? Also when clicking on the "Mount series" button and trying to mount, the mount post request seems to fail.

Yeah that shouldn't be there. Mounting events via Admin UI is not a thing (yet).

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.

Nitpicks below. Looks good generally.

@Arnei
Copy link
Member

Arnei commented Oct 29, 2024

Hm I can't reproduce that. Is this happening every time you try that, even with a freshly started Admin UI, or are there some actions done before that might possibly provoke this?

Was happening consistently for me.

@Arnei
Copy link
Member

Arnei commented Oct 29, 2024

Quick screencap: Bildschirmaufzeichnung vom 2024-10-29 17-25-18.webm

Edit: I now see that my mouse is missing, hope this is clear enough anyway (the last click is on "Homepage")

This adapts the Tobira details component from series details
to be usable in both series and event context.

Note that an Opencast patch (todo: link that patch)
is necessary for this to work.
This was requested by Bern. It is still possible to select
and deselect the path by clicking it's name, but next to that
a checkbox was added for visual clarity.

The checkbox is only shown when the path can be selected
(the page needs to be empty).
Copy link
Contributor

github-actions bot commented Nov 6, 2024

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 ghcr.io/opencast/opencast-admin-interface:pr-878

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 ghcr.io/opencast/opencast-admin-interface:pr-878

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.

Copy link
Contributor Author

@owi92 owi92 left a comment

Choose a reason for hiding this comment

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

I have added a commit that might fix the crashing error you reported, but I can't say for sure.

src/slices/shared/tobiraErrors.ts Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 6, 2024

This pull request is deployed at test.admin-interface.opencast.org/878/2024-11-06_16-20-35/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member

Arnei commented Nov 7, 2024

I have added a commit that might fix the crashing error you reported, but I can't say for sure.

Now crashes with a different error, see below. When creating a news series, this error also occurs immediately upon switching to the tobira tab.

Uncaught TypeError: formik.values.breadcrumbs[(formik.values.breadcrumbs.length - 1)] is undefined
    NewTobiraPage NewTobiraPage.tsx:148
    React 15
    nextPage NewSeriesWizard.tsx:107
    onClick NewAccessPage.tsx:364
    React 15
NewTobiraPage.tsx:148:6

This factors out some code and applies some (yet non-inforced)
linting rules, ES6 syntax and formatting to limit some indentation
levels and improve readability.
Another feature requested by Bern, this adds the necessary
API and UI functions/components to edit existing series paths.

Editing is only allowed if the page with that path has no
other blocks than that of the series.
This pulls the code into a component to limit duplication.
The component can be used when "cancel" and "save" buttons
are needed in modal tabs that allow editing things like
access policies.

This makes it easier to reuse and limits duplicated code.
Most notably the series title (in Tobira context)
was made optional, so we don't have to pass an empty
string to Tobira when it's not specified.
This came up and was suggested in the technical meeting.
It does however mean that the Tobira tab will not really
follow the examplary design-"principles" of the other tabs.
Previously I had them hidden if they couldn't be checked.
After some feedback from Bern we decided to show them anyway, but
add a tooltip to explain why they can't be checked. The disabled
style isn't really that striking or self explanatory.
I hope this will prevent at least some confusion.
503 errors mean that Tobira either isn't configured, or not
configured correcty in Opencast. Unfortunately, it doesn't
distinguish between the two.
Since it might very well be the former, I think we shouldn't
show the error as a notification in the UI.
Instead there will be an info in console saying "Tobira isn't
configured (correctly)" and the Tobira tab will be hidden.
This solution isn't optimal but I think it's the best we
can do for now.

The other errors (404, 500) will trigger notifications that
are shown in the respective Tobira tab.
Something was causing a crash when using the breadcrumb links
to navigate to another page in the path editing menu while
being in "edit mode" (i.e. in the process of adding a new page).
This resulted in a "Maximum update depth exceeded" error, crashing
the interface. The error hints at circular rendering dependendies,
but unfortunately I couldn't reproduce this myself.

But by attempting to do that I noticed another bug that may or
may not be related: The list of children was missing an entry.
I removed the code doing that, since it didn't seem to be necessary
anyway - this also removed a state setting call that might have
caused too many rerenders.
@owi92
Copy link
Contributor Author

owi92 commented Nov 7, 2024

Yeah ok, that I could reproduce. Made a silly code change 🤦 , but should be unrelated to the other crash.

Copy link
Contributor

github-actions bot commented Nov 7, 2024

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 ghcr.io/opencast/opencast-admin-interface:pr-878

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 ghcr.io/opencast/opencast-admin-interface:pr-878

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.

Copy link
Contributor

github-actions bot commented Nov 7, 2024

This pull request is deployed at test.admin-interface.opencast.org/878/2024-11-07_09-33-13/ .
It might take a few minutes for it to become available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tobira Integration
2 participants