fix(admin): render the theme page again with the install-by-name form - #3465
Merged
Merged
Conversation
The "install by name" form on System > Theme bound its name and version fields with la:text. The page is rendered with ThemeListForm, which only has defaultTheme, so LastaFlute failed on the first of them (FormPropertyNotFoundException for "name"). The response had already started, so the browser got a 200 whose HTML stopped at that form, and fess.log logged "Server error: GET /admin/theme/" on every view. Use plain text inputs, as the catalogue rows next to it already do with their hidden inputs. The install action still binds both fields to ThemeInstallForm and validates them there. AdminThemeActionTest now checks that every property a form tag binds in admin_theme.jsp exists on ThemeListForm.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
System > Theme (
/admin/theme/) returns 200 but its HTML stops part-way, at the "install by name" form, with no closing</html>.fess.loglogsServer error: GET /admin/theme/on every view.The form added in #3462 binds its name and version fields with
la:text. The page is rendered withThemeListForm, which only hasdefaultTheme, so LastaFlute fails on the first of them (FormPropertyNotFoundExceptionforname) after the response has already started.What changes
admin_theme.jsp: the install-by-name fields are plain<input type="text">elements, like the hidden inputs of the catalogue rows next to them. The install action still binds both toThemeInstallFormand validates them there.AdminThemeActionTest: a new test checks that every property a form tag binds inadmin_theme.jspexists onThemeListForm. It fails on the current page. The existing install-form test now pins the plain inputs.Verification
mvn test -Dtest=AdminThemeActionTest,MessageTagUsageTest: 37 tests passed. The new test fails without the JSP change.admin_theme.jspwith 0 errors.GET /admin/theme/returns 200 with HTML ending inside the install-by-name form, andServer error: GET /admin/theme/is logged.</html>and nothing is logged. Submitting the form with an unpublished theme name shows the usual "failed to install" message on the full page.