Skip to content

Add support for a custom "home" icon #2696

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

Merged
merged 5 commits into from
May 14, 2025
Merged

Add support for a custom "home" icon #2696

merged 5 commits into from
May 14, 2025

Conversation

Etsija
Copy link
Contributor

@Etsija Etsija commented May 12, 2025

This PR adds a table for storing key-value pairs for UI configuration, and endpoints for getting, inserting and updating the stored configuration values. A minimal UI is implemented as the first use case of the table:

  1. No entries in the configuration table of the database. The UI falls back to showing the "hardcoded" ORT Server favicon as the Home icon:

Screenshot from 2025-05-12 10-42-45

  1. The admin has configured the Home icon by providing a valid URL to it, and toggling the showing of the tailored icon on:

Screenshot from 2025-05-12 10-43-38

  1. The admin can still toggle between the tailored and hardcoded icons in the UI for comparison - this might be important later on, when we add support for more branding elements to the UI:

Screenshot from 2025-05-12 10-43-52

@Etsija Etsija linked an issue May 12, 2025 that may be closed by this pull request
@Etsija Etsija force-pushed the admin-key-value-store branch 3 times, most recently from f7ffaeb to 73dcf09 Compare May 13, 2025 07:51
@Etsija Etsija marked this pull request as ready for review May 13, 2025 07:51
@Etsija Etsija force-pushed the admin-key-value-store branch from 73dcf09 to 2909895 Compare May 13, 2025 10:03
@Etsija Etsija requested a review from sschuberth May 13, 2025 10:11
}
}

HttpStatusCode.InternalServerError to {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return BadRequest instead.

Copy link
Contributor Author

@Etsija Etsija May 13, 2025

Choose a reason for hiding this comment

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

But the endpoint returns InternalServerError when a non-existing key is requested for, so I don't quite understand how I can force that behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

This happens because enumValueOf<ConfigKey> below throws an exception when the key is invalid and you do not catch it, so the catch-all hanlder in StatusPages.kt in core turns it into an internal server error.
Instead, you should catch the exception and return a meaningful error response like so:

val keyParameter = call.requireParameter("key")
val key = runCatching { 
  enumValueOf<ConfigKey>(keyParameter)
}.getOrElse {
  call.respond(HttpStatusCode.BadRequest, ErrorResponse("Invalid config key $keyParameter.", "Allowed keys are: ..."))
  return@get
}

You could also throw an exception that is mapped to BadRequest in StatusPages.kt, but this global error handling does not play well with the idea that component should be self-contained.

Copy link
Contributor Author

@Etsija Etsija May 14, 2025

Choose a reason for hiding this comment

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

Trying to use ErrorResponse in the endpoint leads to a circular dependency warning, so I guess that will also break the self-containment requirement. Isn't core/api's ErrorResponse sitting in wrong location for the components to stay self-contained?
Screenshot from 2025-05-14 06-19-10

Copy link
Contributor Author

@Etsija Etsija May 14, 2025

Choose a reason for hiding this comment

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

I avoided the circular dependency problem by mapping the error response locally to a structure similar to the ErrorResponse class. Also verified that the error toast is rendered properly. While this isn't as elegant as using ErrorResponse directly for typing of the response, I hope it is sufficient for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to move ErrorResponse to :shared:ktor-utils but it can be done in a follow-up.

HttpStatusCode.OK to {
description = "The configuration entry was successfully inserted or updated."
}
HttpStatusCode.InternalServerError to {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return BadRequest instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as before.

@Etsija Etsija force-pushed the admin-key-value-store branch from 2909895 to 6de5614 Compare May 13, 2025 14:39
@Etsija Etsija requested a review from mnonnenmacher May 13, 2025 14:42
@Etsija Etsija force-pushed the admin-key-value-store branch 2 times, most recently from b153d48 to 106d681 Compare May 13, 2025 14:57
Add a table to store key-value pairs which can be used for example for
tailoring the ORT Server UI.

To prevent unwanted insertion of arbitrary keys to the table, the table
key (which acts as the primary key) is specified with an `enum` with
a default value. Extending the table for future needs will be done by
extending the `enum`.

`isEnabled` will be used for the purposes of the UI to
- toggle the specified configuration property on/off to see the effect 
  in the UI
- handle gracefully the case of a missing configuration parameter in the
  database. In this case, the `get()` function returns the default value
  of the `enum` with `isEnabled=false` so the UI can fall back to using
  a default value of the UI element
  
This mechanism will be clarified in further commits.

Signed-off-by: Jyrki Keisala <[email protected]>
@Etsija Etsija force-pushed the admin-key-value-store branch from 106d681 to 918e6cb Compare May 13, 2025 15:01
Etsija added 4 commits May 14, 2025 07:35
In case of the configuration parameter missing in database, 
`getConfigByKey` returns the default value of the enumerated key with
`isEnabled=false`, to be able to fall back to using the pre-configured
default UI element.

`insertOrUpdateConfig` uses `upsert` which adds a new configuration 
entry to the database in case it doesn't exist yet.

The authorization tests are all implemented in a single class for
performance reasons, because the Keycloak testcontainer takes a while to
start.

Signed-off-by: Jyrki Keisala <[email protected]>
In case of the configuration entry for the tailored home icon is 
missing from the database, fall back to showing the pre-configured
default home icon (ORT Server favicon).

Signed-off-by: Jyrki Keisala <[email protected]>
With the form the admin can provide a public URL to the home icon. The
icon can be toggled on/off to see the effect in the UI.

Resolves #2591.

Signed-off-by: Jyrki Keisala <[email protected]>
@Etsija Etsija force-pushed the admin-key-value-store branch from 918e6cb to e263de9 Compare May 14, 2025 04:35
@Etsija Etsija added this pull request to the merge queue May 14, 2025
Merged via the queue into main with commit 61dc0c7 May 14, 2025
30 checks passed
@Etsija Etsija deleted the admin-key-value-store branch May 14, 2025 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to set a custom "home" icon / logo
3 participants