XWIKI-22433: Provide consistency in info / warning / error class usages in templates #3555
Conversation
…es in templates * Part A
…es in templates * Velocity macro update
…es in templates * Part B * extensionHistory.vm * extension.vm * distribution/macros.vm * template/macros.vm * Extensions.xml * AnnotationCode/Macros.xml
…es in templates * Part C * ExtensionSecurity/Admin.xml * edit_macros.vm * LiveTableViewSheet.xml * OfficeImporterAdmin.xml * Annotations/Settings.xml
…es in templates * * Velocity macro update
…es in templates * Part D * job_macros.vm * children.vm * ExtensionBreakingQuestion.form.vm * XClassBreakingQuestion.form.vm
…es in templates * Misc - doubled icon regression
…es in templates * Part E * AttachmentGalleryPickerMacro.java fails with `You must inject a component role. Got [org.xwiki.rendering.internal.macro.message.WarningMessageMacro] at L.87`
…es in templates * Part E * OfficeImporter.xml * XObjectDisplayerProvider.java (WIP) * CreateApplication.xml
…es in templates * Velocity macro update
…es in templates * Part F * ClassEditSheet.xml * CreateWiki.xml (partially tested) * LiveTableEditSheet.xml * macros.vm * editobject.vm (WIP)
…es in templates * Part G * job_status_json.vm * uploadfailure.vm * FilterStreamJobJSON.xml
…es in templates * Part H
…es in templates * Part H
…es in templates * Part I
…es in templates * Part J * Added velocity macro to support the inline mode
…es in templates * Part H - fix
…es in templates * Part F - fix
…es in templates * Part C - fix
…es in templates * Part B - fix
…es in templates * Part F - fix
…es in templates * Part E - codestyle
…es in templates * Part C - fix
…es in templates * Part A - fix
…es in templates * Part G - fix
…es in templates * Part G - fix
| @@ -953,16 +953,44 @@ $html | |||
| ### message | |||
| ### | |||
|
|
|||
| #macro(genericMessage $text $classAffix $iconName $prettyNameKey $isInline) | |||
There was a problem hiding this comment.
This is definitely missing some documentation: be aware that this macro will become public API. It needs docs, since etc. Unless you think it should only be internal, in which case it should be named _genericMessage. But still deserves doc.
There was a problem hiding this comment.
Addressed in 6b9cce1 👍
| <div class="box errormessage xform"> | ||
| {{/html}} | ||
|
|
||
| {{error cssClass="xform"}} |
There was a problem hiding this comment.
Why do we need this xform here? for style only? My understanding is that the goal of the ticket was to simplify the usages of errormessage, so it feels a bit too complex than it should here?
There was a problem hiding this comment.
Why do we need this xform here? for style only?
Yup, I want to minimize the changes in the actual content produced in order to minimize breaking customization and other features. I forgot the exact context here, but I don't want to break the form (style and functionality) just because I removed a class. Having this class is supported by the XWiki macro, so I figured it was okay to refactore things like that.
My understanding is that the goal of the ticket was to simplify the usages of errormessage
The goal of the PR is to factorize their code. This way, next time we update the message boxes, there won't be inconsistencies popping up everywhere (and this also fixes inconsistencies that popped up since we added icons with XWIKI-21452 :) ).
In order to do so, in all of the scripts here, I either use the new velocimacro, or the XWiki macro if possible.
| {{/html}} | ||
|
|
||
| {{error cssClass="xform"}} | ||
| {{html clean="false"}} |
There was a problem hiding this comment.
Hmm and why do we need the clean false here?
There was a problem hiding this comment.
Right before starting the velocity macro, we closed the "current" html macro.
The only use of #maybeShowDeleteUserWarning is a few lines above, in #deleteUserModalContent. In this macro, we can see that the opening HTML block has the parameter clean="false". Since I do not want to break anything, the safest bet is to avoid cleaning the HTML in the content of the error XWiki macro too (It was already in a noncleaned HTML macro before).
There was a problem hiding this comment.
the safest bet is to avoid cleaning the HTML in the content of the error XWiki macro too
that's a safest bet to avoid regression without performing a check, but I'm not sure it's a good solution or a good reflex to have: IMO using clean=false attribute in html macro should only be used for specific identified cases. It's quite possible I'm wrong, but I don't really see what we couldn't use clean true here. IMO it worth using clean true by default, and then checking manually the behaviour. Longer to test, for sure, but better for long term maintenance.
There was a problem hiding this comment.
Note that another potential reason to use clean="false" is performance, since the cleanup can be expensive.
There was a problem hiding this comment.
Hmmm I didn't thought about that. Now I guess better be safe than fast in this specific condition? wdyt?
There was a problem hiding this comment.
Honestly, I don't have a strong opinion on either side. I guess it depends on how complex the content is and how often it's used.
There was a problem hiding this comment.
File using clean="false" or clean=false
|
position | Modified | Reason Step 1 |
|---|---|---|---|
| AdminUsersSheet.xml | inside macro content | Yes (no reason found) |
-- |
| AdminUsersSheet.xml | after macro content | No | backward compatibility of the maybeShowDeleteUserWarning velocimacro. |
| LiveTableEditSheet.xml | inside macro content | Yes (no reason found) |
-- |
| CreateWiki.xml | before macro content ( x2 ) | No | We don't create HTML that is correct inside this macro alone (elements are opened but closed only in another HTML macro). |
| CreateWiki.xml | after macro content ( x2 ) | No | We don't create HTML that is correct inside this macro alone (elements are not opened but closed only here). |
| InvitationCommon.xml | inside macro content | Yes, remove the HTML block |
This is not even HTML syntax (with standard values of translations) |
| InvitationCommon.xml | after macro content | Yes, but just add wiki=false |
We want to set the same HTML macro context as before the macro (see L133). In order to do so, we should have both wiki=false and clean=false |
| ExtensionBreakingQuestion.form.vm | before macro content | We don't create HTML that is correct inside this macro alone (elements are opened but closed only in another HTML macro). | |
| ExtensionBreakingQuestion.form.vm | after macro content | We don't create HTML that is correct inside this macro alone (elements are not opened but closed only here) | |
| ExtensionBreakingQuestion.form.vm | inside macro content | -- | |
| XClassBreakingQuestion.form.vm | before macro content | We don't create HTML that is correct inside this macro alone (elements are opened but closed only in another HTML macro). | |
| XClassBreakingQuestion.form.vm | after macro content | We don't create HTML that is correct inside this macro alone (elements are not opened but closed only here) | |
| XClassBreakingQuestion.form.vm | inside macro content | -- |
| <div class="box errormessage">$errorMessage</div> | ||
| {{/html}} | ||
|
|
||
| {{error}}{{html clean="false"}}$errorMessage{{/html}}{/error}} |
There was a problem hiding this comment.
The clean false doesn't sound accurate here. I don't understand why we'd need it.
There was a problem hiding this comment.
The clean false doesn't sound accurate here. I don't understand why we'd need it.
The clean false was there before. I don't want to break things by removing it.
Also we probably miss escaping
We could add it 👍
This is a change unrelated to refactoring, I'll need to check that it doesn't break anything when adding it.
There was a problem hiding this comment.
The clean false was there before. I don't want to break things by removing it.
I looked further to understand better what this $errorMessage is. It seems to have been copied from ClassEditSheet. The place where it was set in this other template is not anymore in this one, and AFAIU, it's always an empty value so the code we're updating is never reached.
IMO we could probably remove it without it impacting the result of this template in any meaningful way.
AFAICS the content of the errorMessage (if there's some) would be something similar to:
platform.appwithinminutes.classEditorDuplicateFieldNameError=The class has two fields with the same name: {0}. This is plain xwiki syntax, no specific HTML, so we can remove the HTML macro from here, just make sure we escape the translation for XML.
Addressed in 8d2d174 👍
| #end | ||
| #info("$infoFooterContent | ||
| $formContent") |
There was a problem hiding this comment.
hmmm you should really add a comment if the carriage return is on purpose here
There was a problem hiding this comment.
I just checked it again and it doesn't seem to me like this is needed. I just figured out it wouldn't hurt since it's HTML syntax, and it makes it a bit more readable.
| <div class="box successmessage"> | ||
| Done. | ||
| </div> | ||
| #success('Done.') |
There was a problem hiding this comment.
We're missing translations in that file it seems. Might worth adding a comment / creating ticket if there's none.
There was a problem hiding this comment.
Addressed in 87fe4a4 👍
| * Note that the exact context this content is rendered in is not fixed, using the XWiki macro | ||
| * would have ended up in unstable behaviour. | ||
| *# | ||
| #macro (warningWithExtraClass $text $extraClass) |
There was a problem hiding this comment.
Preferable use name starting with _ if it's not supposed to become public. Now I'm a bit surprised that you don't need that elsewhere.
There was a problem hiding this comment.
Now I'm a bit surprised that you don't need that elsewhere.
It's the only place where both:
- I couldn't make it work with XWiki Macro (I tried for an especially long time for this one ^^')
- We needed the
cssClassargument.
Since it's the only place with this very specific need, I figured it wasn't worth creating and exposing a cssClass argument for all the standard macros.
The extra CSS class, even when using XWiki Macro, is rarely used (at least in XWiki Standard).
There was a problem hiding this comment.
Preferable use name starting with _ if it's not supposed to become public.
My bad, addressed in
f75a44c 👍
| ## TODO: we do not escape this string because we inject some HTML code. We should improve it. | ||
| $services.localization.render('admin.ooserver.options.source', ['<span class="monospace">xwiki.properties</span>']) | ||
| </span> | ||
| ## TODO: we do not escape this string because we inject some HTML code. We should improve it. |
There was a problem hiding this comment.
@michitux might be good to have your input on this...
There was a problem hiding this comment.
We have the workaround that is quite verbose that you first get the translation with a parameter placeholder like __VALUE__, apply HTML escaping, and then replace that placeholder with the HTML. For a better solution, we're lacking the APIs that were discussed quite some time ago but not implemented yet.
There was a problem hiding this comment.
Addressed in 4b60a92 👍
Here is what this looks like on my local instance
...rm-web-templates/src/main/resources/templates/job/question/ExtensionBreakingQuestion.form.vm
Outdated
Show resolved
Hide resolved
| @@ -17,15 +17,16 @@ | |||
| ## Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | |||
| ## 02110-1301 USA, or see the FSF site: http://www.fsf.org. | |||
| ## --------------------------------------------------------------------------- | |||
| ##!source.syntax=xwiki/2.1 | |||
There was a problem hiding this comment.
That's a big change... why exactly do you need that? Can't just use the velocity macros?
There was a problem hiding this comment.
Where possible I wanted to still use the XWiki macro, which is still the most standard way. It looked like it was easy enough here to change the default syntax of this template and wrap most things in a velocity macro so that I could use the XWiki macro when needed.
I can revert those changes and use the velocimacro if you confirm that it'd be better 👍
There was a problem hiding this comment.
I don't have anything against moving to xwiki/2.1 in general, but you need to keep in mind that the behavior is very different, especially regarding dealing with new lines and stuff like this. It's most probably more dangerous than you think.
There was a problem hiding this comment.
Reverted the changes.
In order to use the velocimacro here, I had to upgrade the internal one to feature the two secondary parameters of the XWiki macro.
See changes in 161d3d0
Tested on a local instance:
| @@ -17,15 +17,19 @@ | |||
| ## Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | |||
| ## 02110-1301 USA, or see the FSF site: http://www.fsf.org. | |||
| ## --------------------------------------------------------------------------- | |||
| ##!source.syntax=xwiki/2.1 | |||
There was a problem hiding this comment.
Same here, not a big fan of that move...
There was a problem hiding this comment.
Reverted it in 20b86ef 👍
There was a problem hiding this comment.
Manual tests only. I did test most of the changes as much as I could on a local live instance. The changes are across a very large scope. From what I could see, adding a class to the item did not break docker tests.
Frankly given the size of the changes that's not enough. IMO you should at least perform following check:
- ensure that all tests in xwiki-platform-web are passing (we already have PageTests in there)
- ensure that all tests in xwiki-platform-flamingo-skin-test-docker are passing
- you performed changes in AWM / Administration / Extension: I would run the docker tests there too: it's a bit more optional but better be safe.
Not doing this means that there's a high risk we get CI failures and you need to urgently fix stuff...
…es in templates * Updated the velociMacros ** Added comments on the message boxes macros. ** Set the genericMessage as an internal macro.
…es in templates * Added a reference to the newly created issue for translations
…es in templates * Set the warningWithExtraClass velocimacro as internal.
| @@ -17,15 +17,16 @@ | |||
| ## Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | |||
| ## 02110-1301 USA, or see the FSF site: http://www.fsf.org. | |||
| ## --------------------------------------------------------------------------- | |||
| ##!source.syntax=xwiki/2.1 | |||
| {{velocity}} | |||
| #template('job/question/macros.vm') | |||
There was a problem hiding this comment.
If you move to wiki syntax, you should probably move to {{template name="job/question/macros.vm" output="false"/}} (but outside the Velocity macro IMO).
There was a problem hiding this comment.
Not an object anymore since I reverted those changes in regards to #3555 (comment)
| @@ -17,15 +17,19 @@ | |||
| ## Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | |||
| ## 02110-1301 USA, or see the FSF site: http://www.fsf.org. | |||
| ## --------------------------------------------------------------------------- | |||
| ##!source.syntax=xwiki/2.1 | |||
| {{velocity}} | |||
| #template('job/question/macros.vm') | |||
|
|
|||
There was a problem hiding this comment.
Those empty lines are going to cause actual, visual, new lines in the final HTML, which is not what you want.
There was a problem hiding this comment.
Thank you for your help on this. Mostly because of the dangerouseness you pointed out, I decided to revert my changes on both these files and go with the alternative solution of upgrading the velocimacro.
See more at #3555 (comment)
…es in templates * Part C * Removed the TODO and implemented the solution described by Michael
…es in templates * Fixed an inconsistency in the velocimacro compared to the XWiki macro.
…es in templates * Part F * Escaped the errorMessage * removed the useless HTML macro.
…es in templates * Part D * Updated the internal velocimacro to also take into account equivalents to rarely used XWikiMacro parameters.
…es in templates * Part D * Reverted the change of syntax for XClassBreakingQuestion.form.vm
…es in templates * Fixed changes unrelated to the ticket for this PR.
…es in templates * Fixed incorrect HTML context in the content of the error macro.
Jira URL
https://jira.xwiki.org/browse/XWIKI-22433
Changes
Description
Status
I made this table and categories with this order:
.<whatever>message. info--> warning -->error-->success(Table under the
detail dropdown)38 files updated.
Additional file changes
2 files updated
38 + 2 => 40 files total
Screenshots & Video
Invitations.xml
before
after
NotificationAlert.xml
moveStatus.vm
CreateWiki.xml
OfficeImporter.xml
flamingo/macros.vm
Executed Tests
Manual tests only. I did test most of the changes as much as I could on a local live instance. The changes are across a very large scope. From what I could see, adding a class to the item did not break docker tests.
Expected merging strategy