Skip to content

Reduce duplicate code for modal content#1059

Merged
Arnei merged 15 commits intoopencast:mainfrom
Arnei:reduce-duplicate-code-modalcontent
Mar 20, 2025
Merged

Reduce duplicate code for modal content#1059
Arnei merged 15 commits intoopencast:mainfrom
Arnei:reduce-duplicate-code-modalcontent

Conversation

@Arnei
Copy link
Copy Markdown
Member

@Arnei Arnei commented Jan 14, 2025

This code structure appears in like 50 files, so I'm putting it in a component so I don't have to see it again.

While there are many changed lines, 99% of them are indentation changes, so this is hopefully fairly easy to review.

How to test this

Can be tested as usual.

@Arnei Arnei added the type:code-enhancement Internal improvements to the codebase label Jan 14, 2025
@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 ghcr.io/opencast/opencast-admin-interface:pr-1059

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-1059

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
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 14, 2025

This pull request is deployed at test.admin-interface.opencast.org/1059/2025-03-20_12-42-06/ .
It might take a few minutes for it to become available.

Copy link
Copy Markdown
Member

@ziegenberg ziegenberg 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 separate the whitespace changes into a separate commit?

@Arnei
Copy link
Copy Markdown
Member Author

Arnei commented Jan 14, 2025

I can try.

@Arnei
Copy link
Copy Markdown
Member Author

Arnei commented Jan 14, 2025

Although the white space changes are directly related to the introduction of the new component, so it seems reasonable to me to have them in the same commit. Can you explain to me how separating them into their own commit helps with git blame?

@ziegenberg ziegenberg self-requested a review January 14, 2025 16:11
@ziegenberg ziegenberg dismissed their stale review January 14, 2025 16:12

If the white space changes are directly related to the introduction of the new component then leave them as-is.

@github-actions
Copy link
Copy Markdown
Contributor

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

This code structure appears in like 50 files, so
I'm putting it in a component so I don't have to
see it again.
@Arnei Arnei force-pushed the reduce-duplicate-code-modalcontent branch from 28b518a to 47d0018 Compare January 17, 2025 12:47
@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 7, 2025

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

@ferishili ferishili requested review from ferishili and removed request for ziegenberg March 13, 2025 05:26
Copy link
Copy Markdown
Contributor

@ferishili ferishili left a comment

Choose a reason for hiding this comment

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

Thanks @Arnei.
This looks good to me! Since the changes primarily involve replacing the modal HTML with a modal component, I’d appreciate it if you could highlight any areas where additional modifications were made. That way, I can focus my testing on those specific parts. 🚀

@ferishili ferishili self-requested a review March 13, 2025 08:17
Copy link
Copy Markdown
Contributor

@ferishili ferishili left a comment

Choose a reason for hiding this comment

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

I had to add this here to clarify the issue:
I noticed that in User Dialog → Roles List, the modal is not functioning correctly. After reviewing other PRs, I realized this could be a problem in the test environments. However, to ensure this PR is safe, I want to highlight this here first, as it is related to the changes.

@Arnei
Copy link
Copy Markdown
Member Author

Arnei commented Mar 13, 2025

Thanks for the review. Could you give me more information on the issue you are seeing?

  • What exactly is not "functioning correctly" and in which way?
  • What test environment are you using? Test deployment with mock data, docker or a local version of the PR?

@ferishili
Copy link
Copy Markdown
Contributor

Thanks for the review. Could you give me more information on the issue you are seeing?

  • What exactly is not "functioning correctly" and in which way?
  • What test environment are you using? Test deployment with mock data, docker or a local version of the PR?

Of course:
I have tested this on both the PR Test deployment and my local machine. Both have the same issue:

Screenshot 2025-03-13 at 14 27 18

To reproduce:
Navigate to Users (Organization) from left-hand pane > In action column > User details > Go to Roles tab!

@Arnei
Copy link
Copy Markdown
Member Author

Arnei commented Mar 13, 2025

Issues I see:

  • The user name is not displayed in the modal header
  • The overall css of the modal is wonky.

Both of these can be observed on main as well as far as I can tell.

If you are wondering why you cannot add or remove roles, that is likely because the user is marked as "not managable" (a notification on the first tab "User" should tell you as such. That notification should probably be displayed here is well, I'll create an issue for that.) "Not managable" users cannot be edited, so the behaviour is correct in this case.

@github-actions
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@ferishili ferishili left a comment

Choose a reason for hiding this comment

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

The issue I raised has been resolved, so this PR looks good from my side!

@github-actions
Copy link
Copy Markdown
Contributor

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

@Arnei Arnei merged commit c57779c into opencast:main Mar 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:code-enhancement Internal improvements to the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants