feat: Add custom-style support to Djehuty - #275
Conversation
abf9e3e to
50a8ab3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
+ Coverage 18.10% 18.45% +0.34%
==========================================
Files 21 21
Lines 10521 10587 +66
Branches 2040 2059 +19
==========================================
+ Hits 1905 1954 +49
- Misses 8422 8434 +12
- Partials 194 199 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@kikisprdx, can you fix the conflict please :) |
* src/djehuty/web/resources/static/css/main.css: Add CSS variables for the default fonts, and use them in font-family declarations. (cherry picked from commit 59c0d6a) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/config.py: Add fonts attribute as a default None. * src/djehuty/web/ui.py: Add read_fonts_configuration to parse the optional <fonts> block. * src/djehuty/web/wsgi.py: Add fonts.css route and fonts_css handler, and write fonts.css in create_static_error_pages. * src/djehuty/web/resources/html_templates/fonts.css: Add CSS font template. * src/djehuty/web/resources/html_templates/layout.html: Link fonts.css after main.css so instance typography overrides the defaults. (cherry picked from commit 212ead1) Signed-off-by: Kikis <kikispcmr@gmail.com>
* etc/djehuty/djehuty-dev-config.json: Add fonts configuration. * src/djehuty/web/config/json_parser.py: Add method to support repeated config entries (e.g. multiple fonts). (cherry picked from commit 6e3cc21) Signed-off-by: Kikis <kikispcmr@gmail.com>
Custom footers are inserted as raw HTML through the config file, so an inline <style> cannot carry a nonce and is refused by the security policy. Instances can now point at a stylesheet served from their own assets root. * src/djehuty/web/config/runtime.py: Hold the configured stylesheets. * src/djehuty/web/ui.py: Read the stylesheets from the config, and warn on start when a font or a css cannot be resolved. * src/djehuty/web/wsgi.py: Pass the stylesheets on to the templates. * src/djehuty/web/resources/html_templates/layout.html: Include the stylesheets after the defaults so an instance can override them. * src/djehuty/web/config.py: Remove unecessary code. * etc/djehuty/djehuty-dev-config.json: Serve the development fonts from the assets root. (cherry picked from commit 710256f) Signed-off-by: Kikis <kikispcmr@gmail.com>
* etc/djehuty/djehuty-dev-config.json: Add path for custom favicon (cherry picked from commit 2f56761) Signed-off-by: Kikis <kikispcmr@gmail.com>
* etc/djehuty/djehuty-dev-config.json: Load the instance stylesheets, and set the footer to the dark grey. (cherry picked from commit cd02345) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/resources/html_templates/portal.html: Add margin botton for the new-notice. (cherry picked from commit 5a4c411) Signed-off-by: Kikis <kikispcmr@gmail.com>
* etc/djehuty/djehuty-dev-config.json: Update the development config to include the new chip and shape styles. (cherry picked from commit 926beba) Signed-off-by: Kikis <kikispcmr@gmail.com>
* etc/djehuty/djehuty-dev-config.json: Load the new stylesheet. (cherry picked from commit c15b216) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/resources/html_templates/public_metadata.html: Remove the dots from the Export as. (cherry picked from commit 19e3e60) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/resources/html_templates/dataset.html: Write the data headings in sentence case, so the wording no longer carries the styling. (cherry picked from commit 70759dd) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/resources/html_templates/portal.html: Rename categories with an ampersand. (cherry picked from commit a902abe) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/resources/html_templates/colors.css: Increasing space between files and button. (cherry picked from commit d1f604e) Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/web/ui.py: Ignore empty custom stylesheet paths. Signed-off-by: Kikis <kikispcmr@gmail.com>
* tests/unit/test_json_clean_schema.py: Test repeated JSON configuration
values through `findall`.
* tests/unit/test_config_parser.py: Test font and stylesheet configuration
parsing and asset validation.
* tests/unit/test_theme_css.py: Test generated font stylesheet rendering.
* tests/e2e/tests/test_smoke.py: Test that the generated font stylesheet
is served and linked from portal pages.
Signed-off-by: Kikis <kikispcmr@gmail.com>
…alues. * etc/djehuty/djehuty-dev-config.json: Add placeholder config examples. * etc/djehuty/djehuty-example-config.json: Add placeholder config fields as examples. * etc/djehuty/djehuty-example-config.xml: Add placeholder config fields as examples. * src/djehuty/web/ui.py: Add guard for the case that no source is found. Protects under the case of defined CSS variables but empty values. Signed-off-by: Kikis <kikispcmr@gmail.com>
* tests/e2e/tests/test_smoke.py: Stop requiring a font-face declaration when
no fonts are configured.
`TestFontsCss::test_renders_font_faces_and_variables` covers rendered font.
* tests/unit/test_config_parser.py: Fix PEP-8 formatting.
* tests/unit/test_theme_css.py: Fix PEP-8 formatting.
Signed-off-by: Kikis <kikispcmr@gmail.com>
* tests/e2e/tests/test_citation.py: Alter test to assert against the new UI text "Export as", which previously was "Export as...". Signed-off-by: Kikis <kikispcmr@gmail.com>
50a8ab3 to
b251948
Compare
641e16
left a comment
There was a problem hiding this comment.
Thank you for splitting this out, the splitting is done well! Unit tests plus an e2e check for the generated stylesheet is also super nice!
There are a few things implementation wise that I think should be fixed before a merge:
- fonts.css values are HTML-escaped.
- mono-font-family does nothing; Nothing in the codebase reads var(--font-mono).
- assets_root is None misses "", which is what both example configs ship and reports a misleaing message.
- The theme-CSS tests can't catch the first bullet point issue.
- Docs not included. In general I feel like docs for a feature or an implementation as such should be shipped together in one PR. just a nit.
Some other nits I saw:
- I don't think the new findall() handles nested paths nor the [@attr='value'] predicates that find() supports, returning [] rather than erroring.
- --font-ui is read by one selector (#primary-menu li a), which is narrower than the name suggests.
One note for future PRs:
src/djehuty/web/ui.py isn't in the [tool.ruff] include list, so running just format over it produced a lot of changes which makes it hard to find the actual feature code. Either keep formatting to the paths already in the include list or put reformats in their own commits. Otherwise, great work!
| #new-notice { margin-top: 0em; margin-bottom: 40pt } | ||
| </style> |
There was a problem hiding this comment.
isn't this missing a ;?
| #new-notice { margin-top: 0em; margin-bottom: 40pt } | |
| </style> | |
| #new-notice { margin-top: 0em; margin-bottom: 40pt; } | |
| </style> |
otherwise, although these change the UI slightly, these seem as generic UI polishes so I think good to keep in this PR and not part of some specific seperate branding.
| try: | ||
| from onelogin.saml2.auth import OneLogin_Saml2_Auth # pylint: disable=unused-import | ||
| from onelogin.saml2.errors import OneLogin_Saml2_Error # pylint: disable=unused-import | ||
|
|
There was a problem hiding this comment.
On the ui.py formatting: no need to change anything in this PR now, but a note for future ones. src/djehuty/web/ui.py isn't in the [tool.ruff] include list in pyproject.toml yet (its a draft PR #185), so running just format over it produces a big formatting diff that isn't enforced afterwards. That makes the lines of actual feature code hard to find during review, and it doesn't stick (e.g., the recent rebase already pulled old-style call sites back in from main), so the file is currently a mix of both styles and ruff format --check still flags it.
For future PRs: either keep formatting to the paths already in the include list, or put the reformat in its own commit so the feature diff stays readable. Preferably option 1.
| "privilege-button-color": "#fce3bf", | ||
| "footer-background-color": "#707070" | ||
| }, | ||
| "fonts": { |
There was a problem hiding this comment.
Maybe it would be helpful to have an actual filled in example for the config?
Same applies to the xml exmaple file.
| environment = Environment(loader=FileSystemLoader(TEMPLATES_DIR)) | ||
| template = environment.get_template("fonts.css") |
There was a problem hiding this comment.
This builds its own Environment without autoescape, so it renders the template under different rules than the server does (wsgi.py:440 passes autoescape=True). The assertions below pass on output that production never produces, which is how the escaping issue in fonts.css got through. Worth having the test construct its environment the same way WebServer does.
There was a problem hiding this comment.
Thank you, this was a good catch!
| font-family: '{{face.family}}'; | ||
| src: url('{{face.src}}') format('{{face.format}}'); |
There was a problem hiding this comment.
These values go through Jinja's HTML autoescaping, because the environment that renders this template is built with autoescape=True (wsgi.py:440). CSS doesn't understand HTML entities, so a family name containing an apostrophe renders as font-family: 'Bob's Sans' - valid CSS syntax naming a font that doesn't exist, so it never loads. Also any src with a query string gets its & turned into &, changing the URL that's actually requested.
Adding | safe is not going to fix it. The three --font-* variables below already have it, and --font-body is broken too: an apostrophe in an admin-supplied value ends the CSS string early and the browser discards the whole declaration. Family is being template-quoted while body_font is admin-quoted.
On a dev instance: setting "family": "Bob's Sans" and "body-font-family": "'Bob's Sans', sans-serif" (restart the container) and check http://localhost:8080/theme/fonts.css :
@font-face {
font-family: 'Bob's Sans';
src: url('/assets/f.woff2?v=1&x=2') format('woff2');
font-display: swap;
}
:root {
--font-body: 'Bob's Sans', sans-serif;
}
There was a problem hiding this comment.
This is a good catch! This is an issue impacting all CSS config options, but for hex colours, we never had to deal with this possibility.
| --font-mono: 'FiraMono', monospace; | ||
| } |
There was a problem hiding this comment.
An admin who sets "mono-font-family": "'JetBrains Mono', monospace" gets the value parsed correctly, served correctly, and sitting in their browser correctly but nothing on the page changes. This is because nothing seems to read --font-mono; the places that do render monospace text name the font directly instead of going through the variable. In portal.html:11, admin/exploratory.html:19 and :27 all say font-family: 'FiraMono', monospace;.
Because the default --font-mono: 'FiraMono', monospace happens to match what the hardcoded rules already use, the site looks correct. The gap only shows up when someone tries to change it which is the one thing the feature exists to allow from what I understand.
| def read_fonts_configuration(xml_root): | ||
| """Procedure to parse and set the custom typography configuration.""" | ||
| fonts = xml_root.find("fonts") | ||
| if fonts is None: | ||
| return | ||
|
|
||
| font_faces = [] | ||
| for face in fonts.findall("font-face"): | ||
| font_faces.append( | ||
| { | ||
| "family": config_value(face, "family"), | ||
| "src": config_value(face, "src"), | ||
| "format": config_value(face, "format", fallback="woff2"), | ||
| "weight": config_value(face, "weight"), | ||
| "style": config_value(face, "style"), | ||
| "display": config_value(face, "display", fallback="swap"), | ||
| } | ||
| ) | ||
|
|
||
| config.fonts = { | ||
| "font_faces": font_faces, | ||
| "body_font": config_value(fonts, "body-font-family"), | ||
| "ui_font": config_value(fonts, "ui-font-family"), | ||
| "mono_font": config_value(fonts, "mono-font-family"), | ||
| } |
There was a problem hiding this comment.
No fallback= anywhere; config.fonts = {...} doesn't update config.fonts, it throws the old one away and puts a new dictionary in its place. Anything this file didn't specify becomes None, and font_faces becomes an empty list if this file has no font-face entries. Give config.fonts a real default in runtime.py, then merge per key like colours (function above: read_colors_configuration) does. You need to also decide if qhen a second file has entries, should they be added to the parent's list, or replace it? this is a design choice.
Maybe also drop incomplete faces in read_fonts_configuration rather than in the template, so the validator and the renderer agree on what is valid, and identify entries by index in the warning so the message is actionable.
There was a problem hiding this comment.
I think it's fine to have no fallback. It would be easier to spot when a font rule isn't working properly. In the case of multiple files, default behavior would just use the latest definition, which I think makes sense for configuration files. I agree with the validator change, it would make troubleshooting easier.
| if assets_root is None: | ||
| logger.warning("%s refers to '%s' but 'custom-assets-root' is unset.", description, source) | ||
| elif not os.path.isfile(os.path.join(assets_root, source[len(prefix) :])): |
There was a problem hiding this comment.
is None misses the empty-string case, so this disagrees with the check at line 1146. config_value returns "" (not None) for "custom-assets-root": "", which is what both djehuty-example-config.json and djehuty-dev-config.json use.
| "family": "Example Sans", | ||
| "src": "/assets/fonts/example-sans.woff2", | ||
| "format": "woff2", | ||
| "weight": "400 700", | ||
| "style": "normal", | ||
| "display": "swap", | ||
| }, | ||
| ], | ||
| "body_font": "'Example Sans', sans-serif", | ||
| "ui_font": "'Example Sans', sans-serif", | ||
| "mono_font": "'Example Mono', monospace", |
There was a problem hiding this comment.
There isn't a single character in there that HTML escaping would touch. No apostrophe, no ampersand, no angle bracket. Escaping "Example Sans" gives you back "Example Sans". Also could be how the escaping issue got through.
There was a problem hiding this comment.
I'll add some new regressions tests for this.
* etc/djehuty/djehuty-example-config.json: Add placeholder font values. * etc/djehuty/djehuty-example-config.xml: Add placeholder font values. Signed-off-by: Kikis <kikispcmr@gmail.com>
* src/djehuty/utils/convenience.py: Add new `css_string` filter, which escapes apostrophes and back slashes. * src/djehuty/web/resources/html_templates/fonts.css: Pipe css variables through `css_string`. * src/djehuty/web/wsgi.py: Register `css_string` as a Jinja filter. * tests/unit/test_theme_css.py: Register `css_string` as a Jinja filter for tests. Signed-off-by: Kikis <kikispcmr@gmail.com>
… missing font paths with more specific logging warnings. * src/djehuty/web/resources/html_templates/admin/exploratory.html: Replace hardcoded font with css variable. * src/djehuty/web/resources/html_templates/portal.html: Replace hardcoded font with css variable, and minor CSS formatting fixes. * src/djehuty/web/ui.py: Warn about an unresolvable asset when the assets root is empty (not just unset). Drop `font-face` entries missing `family` or `src` during config parsing instead of letting the template render invalid CSS for them and identify font entries by index in warnings. Signed-off-by: Kikis <kikispcmr@gmail.com>
* tests/unit/test_config_parser.py: Pass `logger` through to
`read_fonts_configuration`; add coverage for the empty-`assets_root`
warning and for dropping incomplete font-face entries.
* tests/unit/test_theme_css.py: Add coverage asserting font values are not
HTML-escaped, since `fonts.css` output is CSS, not HTML.
Signed-off-by: Kikis <kikispcmr@gmail.com>
Thank you for your review and good eye! I've solved the outstanding issues. A few points I think would be appropriate for a future refactor. Matching the |
|
I've checked the latest changes, it seems good. |
Hi @kairoaraujo, yes I will do a quick final round today :) |
641e16
left a comment
There was a problem hiding this comment.
Thanks for working through these. I've checked the fixes and they all do what they say, good job!
A few new things slipped through in with these commits so please have a look!
| "custom-assets-root": "/etc/djehuty/assets", | ||
| "custom-stylesheet": [ | ||
| "/assets/css/palette.css", | ||
| "/assets/css/buttons.css" | ||
| ], | ||
| "custom-logo-path": "/etc/djehuty/assets/images/logo.png", | ||
| "custom-favicon-path": "/etc/djehuty/assets/images/favicon.ico", |
There was a problem hiding this comment.
These paths don't exist, and the example config is the file people copy to get started.
Copying this config would result in your site losing its logo and favicon with no explanation.
The old empty-string values had their own problem, but that's fixed in the code now. I'd suggest leaving these keys out of the example entirely and letting the docs in #283 carry the worked example. Or perhaps you can add comments to these lines to indicate the logo and favicon should be added to these directories for the template to work as necessary. Same applies to the XML version.
There was a problem hiding this comment.
Given that we don't have anything to supply as default variables (as default styling is part of the source code) I think the current approach should be clear. I'll add comments to make it clearer these are placeholders.
| for index, face in enumerate(config.fonts["font_faces"]): | ||
| warn_about_unresolvable_asset( |
There was a problem hiding this comment.
These index numbers don't match the ones in the drop warning, because this counts the filtered list while line 716 counts the original one.
With a bad face first and a good one second:
WARNING: Dropping font-face #0: 'family' and 'src' are both required.
WARNING: Font 'Good' (font-face #0) refers to '/assets/fonts/good.woff2' which does not exist under '...'
Two different entries both reported as #0. Since the number is there to tell entries apart, it's misleading when there are several with problems. Keeping the original index on each face when it's built would sort it out I think..
| "display": None, | ||
| }, | ||
| ], | ||
| "body_font": "'O'Brien & Sons', sans-serif", |
There was a problem hiding this comment.
This value is broken CSS, and the assertion on line 103 makes it seem like this is the expected output. Using a properly escaped stack like 'O\'Brien & Sons', sans-serif tests exactly the same behaviour..
The family value here is correct though -> the template supplies the quotes, css_string escapes the apostrophe, and font-family: 'O'Brien & Sons'; is correct.
body_font on line 95 is the one I'd change. That field the admin must writee the quotes themselves and it goes through bare | safe which means 'O'Brien & Sons', sans-serif is a malformed value: the string closes after 'O' and the browser discards the declaration. Line 103 then asserts that broken output is expected.
The assertion is checking that we don't HTML-escape the stack, and a correctly-quoted value proves that just as well. the & is still unescaped, so the & guard keeps working:
so something like:
"body_font": "'O\'Brien & Sons', sans-serif",
assert "--font-body: 'O\'Brien & Sons', sans-serif;" in css
|
|
||
| def css_string(value): | ||
| """Escape VALUE for safe interpolation into a single-quoted CSS string.""" | ||
| return str(value).replace("\\", "\\\\").replace("'", "\\'") |
There was a problem hiding this comment.
Worth also handling newlines here. A CSS string can't span lines, and config_value returns element text without stripping it, so ordinary XML formatting like:
<family>
Example Sans
</family>
renders a CSS string with real line breaks in it, which the browser won't parse. Either strip the value or escape \n as \A. read_custom_stylesheets already strips its entries, so it would be nice to follow the same pattern here.
…iltering fonts. * src/djehuty/utils/convenience.py: Replace new lines with an escape sequence. * src/djehuty/web/ui.py: Strip whitespace from config values. Store index, and re-use it for logging. Signed-off-by: Kikis <kikispcmr@gmail.com>
…ixes. * tests/unit/test_config_parser.py: Cover config_value stripping leading and trailing whitespace from XML text nodes. * tests/unit/test_convenience.py: Cover css_string escaping backslashes, quotes, and new lines. * tests/unit/test_theme_css.py: Fix body_font test value to use a double-quoted CSS string instead of requiring backslash-escaping. * tests/unit/test_theme_css.py: Add linter suggestion. Wrap in `'` to avoid escaping. Signed-off-by: Kikis <kikispcmr@gmail.com>
8fcac3c to
930bf32
Compare
* tests/unit/test_config_parser.py: Add index to dictionaries in `test_reads_font_faces_and_font_families`. Signed-off-by: Kikis <kikispcmr@gmail.com>
641e16
left a comment
There was a problem hiding this comment.
Thank you for your changes :) Looks good now!
Summary
Add support for loading custom fonts, CSS stylesheets and static
assets from Djehuty's JSON configuration.
Changes
src/djehuty/web/config/json_parser.py: Support repeated JSON definitionsthrough
findall.src/djehuty/web/config/runtime.py: Store custom font and stylesheetconfiguration at runtime.
src/djehuty/web/ui.py: Read font and stylesheet configuration and validateconfigured asset paths.
src/djehuty/web/wsgi.py: Serve generated font CSS and configured themeassets.
src/djehuty/web/resources/html_templates/fonts.css: Generate font-facedeclarations and typography variables.
src/djehuty/web/resources/html_templates/layout.html: Load generated fontCSS and instance-specific stylesheets.
src/djehuty/web/resources/static/css/main.css: Apply configurabletypography variables.
etc/djehuty/djehuty-dev-config.json: Add empty custom-styleconfiguration fields.
etc/djehuty/djehuty-example-config.json: Add empty custom-styleconfiguration fields.
etc/djehuty/djehuty-example-config.xml: Add empty custom-styleconfiguration fields and a font-face definition.
tests/unit/: Test JSON parsing, theme configuration, asset validation andgenerated CSS.
tests/e2e/tests/test_smoke.py: Test that the generated font CSS is served andlinked from portal pages.
Approval Checklist
Issue Reference (optional - PRs may not be associated with an issue)
Closes #237
Screenshots (optional)
Notes (optional)
I changed
read_custom_stylesheetsto ignore blank custom stylesheet entries in the config. See 404fd8e. I changedread_custom_stylesheetsto ignore blank custom stylesheet entries and added a warning for configured assets without a source (f799fd9). All other code from @gabrielakuhn remains unchanged.All example configs have fields defined to demonstrate usage. The
djehuty-dev-config.jsonalso has fields for consistency.