Modernize the UI and add preselect URLs#681
Conversation
updating branch with the master changes
PierreBtz
left a comment
PierreBtz
left a comment
There was a problem hiding this comment.
Thanks for taking a stab at this. Having profiles to simplify component selection has been a long awaited feature!
I didn't look in details yet, but I have a couple of questions:
-
it looks like you chose to go with a client only implementation, which means that we won't have this feature available in the API for use by a external tool. Did you consider implementing the feature server side?
-
I'm worried about delegating the hash implementation to all the plugin developers offering a
Component, which excludes important components from this new feature (and might be seen by end users as a bug in this plugin). Maybe we could thing of another hash implementation that would work out of the box for anybody (something based on the className of the component maybe?).
|
Hi Pierre, thank you for taking care of that.
I did not consider it. The development of having a server-side should be easy. Initially, we can have an input hidden textbox in the form that the server side will ignore if the rest of the names/checkboxes are present in the data (which will mean that we are coming from the UI), and it will be treated in case the other elements are missing (so, we can use this value for APIs).
That is an excellent question. Cristian and I have been discussing this question in depth. AFAIU, including the The main goal of this development (apart from making the generation of support bundles easier for the users) is to have some previously discovered codes/hashes. Once a user has a problem, we (the community) can share one of those codes/hashes with the user to accurately collect the information that we need to help with the troubleshooting. If the user has a set of plugins (that we don't know yet until having the first support bundle) that include new In the current approach (ignoring the new It is true that if a developer introduces a new component and uses the same hash that already exists in the |
|
Hi @PierreBtz, I have been reviewing the options for the "I'm worried about delegating the hash implementation to all the plugin developers offering a Component" problem. I do believe the best option to avoid this problem (other developers could break the new system making wrong implementation of the getHash method) is to have a list in the What do you think? |
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
👎 on the hard-coded list of components from a modularity perspective. Components provided by other plugins should be treated as first class.
The custom options and hash system seems like a weird UX to me. Why not just remember the last-selected set of components in a UserProperty and use that as a default, unless you reset to a predefined profile? https://issues.jenkins.io/browse/JENKINS-20904 681606c#commitcomment-4724976
Also see this CloudBees-internal issue.
|
Also I wonder if we need the dropdown and the popup, or if we could just have buttons to select all/none of the components generally, or within one of the categories. That would be make it easy to collect all components pertaining to agents, for example. Plus a button to reset to defaults. |
|
@ironcerocloudbees sorry for the delay. My suggestion to get going would be to split this development in two since you are trying to tackle two issues at once:
The current architecture is problematic, the plugin shouldn't have knowledge of downstream components. I tend to agree with Jesse's approach of storing custom profiles as a UserProperty (or if want to keep things on the front end, a cookie maybe?). One thing that this approach doesn't allow is custom profile sharing (I don't know if it's a requirement of yours or not?). If it is, you could maybe provide an url fragment to share with the component selection appearing as query parameters. Note that to keep things simple, this can be delivered independently on a subsequent PR.
No strong opinion on this I'm no UX designer (and maybe we could ask for the opinion of one...). I personally like that the main view is not polluted by a long list of components anymore. |
I like this idea. There is already a |
jglick
dismissed
their stale review
No longer hard-coding components that I can see.
There was a problem hiding this comment.
Thanks for the contribution and sorry for reviewing so late...
Overall it looks OK to me but there is still some work to it until we can merge. I believe the Component ID approach is best due to the plugin extensions. FWIW I think the UI / CLI could be adapted to support selecting a profile.
Also I wonder if we need the dropdown and the popup, or if we could just have buttons to select all/none of the components generally, or within one of the categories. That would be make it easy to collect all components pertaining to agents, for example. Plus a button to reset to defaults.
I think this was general idea we had the various time we discussed about this. I was imagining a reactive grid component with profile selection and even single component content download. But my expertise in UI / fronted is limited. The feature in this PR is still a step forward towards this and that's great.
Couple of points I thin kneed to be addressed:
-
When testing this PR, I was looking for the list of components that each of those options would pre-select. Which I could not get. I thought that the "Select components" would show me the list for the option I just picked but actually it doesn't. I wish I could see what components are included.
-
The default selection is changed. There is a method
isSelectedByDefault()that should be integrated in the "Default" options to preserve it. NOTE: the review of the defaut selection is a topic in itself. There's been a growing lists of components that are selected by default and that need to be reviewed. Maybe as part of a different PR. -
Some UI tests will need to be updated I think.
NOTE: I am also wondering if the options list should be made extensible. If other plugin could contribute a profile for example.
| public String getApplyCustomChanges() { | ||
| return Messages.SupportPlugin_ApplyCustomChanges(); | ||
| } | ||
|
|
||
| public String getChooseGeneralComponent() { | ||
| return Messages.SupportPlugin_ChooseGeneralComponent(); | ||
| } | ||
|
|
||
| public String getChooseGeneralComponentApplied() { | ||
| return Messages.SupportPlugin_ChooseGeneralComponentApplied(); | ||
| } | ||
|
|
||
| public String getChooseGeneralComponentApplying() { | ||
| return Messages.SupportPlugin_ChooseGeneralComponentApplying(); | ||
| } | ||
|
|
||
| public String getChooseComponents() { | ||
| return Messages.SupportPlugin_ChooseComponents(); | ||
| } | ||
|
|
||
| public String getClose() { | ||
| return Messages.SupportPlugin_Close(); | ||
| } | ||
|
|
||
| public String getGenerateSupportBundle() { | ||
| return Messages.SupportPlugin_GenerateSupportBundle(); | ||
| } | ||
|
|
||
| public String getGenericComponentConfigurationFiles() { | ||
| return Messages.SupportPlugin_GenericComponent_ConfigurationFiles(); | ||
| } | ||
|
|
||
| public String getGenericComponentCopy() { | ||
| return Messages.SupportPlugin_GenericComponent_Copy(); | ||
| } | ||
|
|
||
| public String getGenericComponentCustom() { | ||
| return Messages.SupportPlugin_GenericComponent_Custom(); | ||
| } | ||
|
|
||
| public String getGenericComponentDefault() { | ||
| return Messages.SupportPlugin_GenericComponent_Default(); | ||
| } | ||
|
|
||
| public String getGenericComponentPerformanceData() { | ||
| return Messages.SupportPlugin_GenericComponent_PerformanceData(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Since those are only used in Jelly and not dynamically set, they could just be added to the index.properties maybe.. For the description of the Options (enum) see below.
There was a problem hiding this comment.
Done.
I didn't know that we used to use the index.properties for "static" values. I already move the static ones to the index.properties.
Thanks for the tip.
| public enum PreChooseOptions { | ||
| Default, | ||
| ConfigurationFiles, | ||
| PerformanceData | ||
| } |
There was a problem hiding this comment.
I suggest that we enrich the enum to hold the internationalization. Also seems to me that custom should be included in the options ?:
public enum PreChooseOptions {
Default(Messages._SupportPlugin_GenericComponent_Default()),
ConfigurationFiles(Messages._SupportPlugin_GenericComponent_ConfigurationFiles()),
PerformanceData(Messages._SupportPlugin_GenericComponent_PerformanceData()),
Custom(Messages._SupportPlugin_GenericComponent_Custom());
private final Localizable description;
PreChooseOptions(Localizable description) {
this.description = description;
}
public Localizable getDescription() {
return description;
}
}
There was a problem hiding this comment.
Done.
I created the localization directly in the enum. Previously, I already had it using a generic method in the SupportAction class. However, this implementation looks more elegant to me :)
| public String getPreChooseOptionsMessage(PreChooseOptions preChooseOptions) { | ||
| ResourceBundleHolder holder = ResourceBundleHolder.get(Messages.class); | ||
| return holder.format("SupportPlugin_GenericComponent_" + preChooseOptions.name()); | ||
| } |
There was a problem hiding this comment.
Should not be needed if the enum holds the description.
| public SupportAction.PreChooseOptions[] getDefautlPreChooseOptions(){ | ||
| return new SupportAction.PreChooseOptions[0]; | ||
| } | ||
|
|
||
| public Boolean checkDefaultPreChooseOptions(SupportAction.PreChooseOptions preChooseOptions) { | ||
| return Arrays.stream(getDefautlPreChooseOptions()).anyMatch(it -> {return it == preChooseOptions;}); | ||
| } |
There was a problem hiding this comment.
In a way, it might be simpler to just have the boolean isApplicable(SupportAction.PreChooseOptions preChooseOptions) and for component to just override it. Otherwise we are doing an Arrays.stream for every single component.
Now as far as I understand, we are actually changing the default selection here. Some components are selected by defaults - see isSelectedByDefault() - so in the case of the Default option we could use return isSelectedByDefault() and for other cases let the actual component override the method.
There was a problem hiding this comment.
Done,
I changed the stream/array for a method (boolean isApplicable(SupportAction.PreChooseOptions preChooseOptions) that gets the information from the isSelectedByDefault(), and we can easily override it if we need to add the component to a performance data or any other pre-choose options set.
| You can also select the <i>Custom</i> component and type a hexadecimal value in the auxiliary text box if you want to check some specific components (the components will be checked depending on this hexadecimal value). To get an example of the hexadecimal value, you click in the <i>Select components</i>> button, mark the components that you want and copy the value that you can find in the bottom and right corne of the widget. | ||
| </p> |
There was a problem hiding this comment.
This paragraph will need to be updated if we don't use the hash
|
Hi @Dohbedoh, Thank you so much for your review.
This behavior sounds like a bug. When we choose an element in the dropdown menu (with Could you please check if you have any JS errors in the browser console?
That is a really good idea. I will modify it to use the
Are you considering additional tests for the UI changes? Or did you mean revisiting the UI tests with the new UI changes in the PR?
Are you meaning the predefined option lists (aka For the rest of the comments/tips/etc, I will update the sub-conversations. Kind regards, |
| return false; | ||
| } | ||
|
|
||
| public SupportAction.PreChooseOptions[] getDefautlPreChooseOptions(){ |
jglick
changed the title
…he options localizable directly in the enum
4 participants
This is an attempt to enhance the Support Bundle Generation view. The improvements introduced are:
Moving the list of options from the main page to a pop-up/widget.
Users don't always need to mark or unmark any options if they just want to generate a default support bundle. Moving the list to a pop-up or widget allows us to add more elements to manage the checked options in the main view (see next improvement sections).
To open the widget we will need to click on
Select options:The new widget shows the following:
Please pay attention to the code displayed in the bottom-right corner. This is a HASH generated from the current selected (checked) options. Please refer to the "Custom options" section for more information about this new element.
You can close the widget by clicking on
Closeor pressing theEsckey.Choose a general set of options
Selecting which options should be checked can be complex for standards users, so it may be a good idea to have a dropdown element with some standard options (such as configuration files, performance, etc.). For the moment, only three options were added (default, default + configuration files, and performance). As soon as the user clicks or selects one general option in this dropdown, the options will be checked in the widget (by JS).
Custom options
This could be the most interesting part of this development.
Each option will have an ID in the list of options. Anytime that a user check or uncheck any option in the widget a HASH value will be calculated in base on the current selected options. This HASH will appear in two locations: in the bottom-right corner of the widget, and in the input textbox under the
Choose a general set of options.Selecting the
Customvalue in theChoose a general set of optionsdropdown, the textbox for the custom value will appear. If you type a HASH value in this textbox and press theEnterkey (or click in theApplybutton), the options in the widget will be selected according to this HASH value. After that, we could optionally click onSelect optionsand alter this set of select options.There is an issue with plugins that extend the
Componentclass. The elements shown in the widget (and originally on that page) are classes that extend theComponentclass and satisfy some requirements. Those classes can be provided by any plugin (not only the support-core-plugin). This question makes the management of this HASH so hard (since we cannot ensure that all the child components will implement thegetHashmethod (inherited from theComponentclass). Therefore, the decision was made to disregard the child components that would not correctly implement thegetHashmethod. By doing that, we can share a HASH code that we can ensure will work.Testing done
The development was tested in three different ways:
getHashmethod of the child components.2.479.3and passed the basic integration tests:Customgeneral options and confirming that the correct options were checked with different known HASH codes.Escfor the widget or theEnterfor the custom textbox).Submitter checklist