Bug 26207

*It* could be useful. But yes, I could be useful too I guess.

+1

Created attachment 108271 [details] [review]
Bug 26207: Compare values of system preference log entries

This patch adds the ability to compare the values of system preferences
in the system log. The user can select two entries in the log search
results and view a diff of the two versions.

The feature is designed specifically for CSS and JS preferences like
OpacUserCSS or OpacUserJS where code changes are difficult to track.

To test, apply the patch and go to Tools -> Log viewer. You may need to
add values and make changes to preferences like OpacUserCSS if you don't
already have a history in your logs.

- Perform a search for log entries in the "System prefs" module.
- In the results, each entry for a system preference should have a
  "Compare" checkbox."
- Check one of the checkboxes.
  - The table should now be filtered by the system preference name. This
    facilitates meaningful selections for comparison.
  - You should see a message displayed onscreen, "Showing results
    for...[ preference name]"
  - The "Check none" link in the toolbar above the table should now be
    enabled.
  - If youy click the "Compare selected" link in the toolbar at this
    stage you should get a message, "You must select two entries to
    compare."
  - If you uncheck the checkbox the table should return to an unfiltered
    state.
- Check two checkboxes.
  - The "Compare selected" link in the toolbar should now be enabled.
  - Next to each of the checkboxes you checked should be a "View
    comparison" link.
  - Clicking either of the "View comparison" links or the "Compare
    selected" link should trigger a modal with the diff view of the two
    preferences.
- Try to select a third checkbox. You should get a message, "You can
  select a maximum of two checkboxes." The box should remain unchecked.
- Test that unchecking both checkboxes manually clears the table filter.
- Test that the "Check none" link works to uncheck checked boxes and
  clear the table filter.

The patch worked well and I will find it very useful.  

Here are some observations from testing:

For libraries that have a lot of CSS or jQuery it would be helpful to add a horizontal scroll bar at the top of the modal.

I think it would be preferable to have the first line of the modal left justified - right now it is indented.  

Also, it would be nice to have the name of the system preference (such as IntranetUserJS) on the first line of the modal with the first line of actual code displaying on a separate line - right now the first line of code displays after the system preference name and it runs off the page causing the user to have to scroll.

Removing code worked just fine.  Copying a section of code and moving to another place in the system preference would split comments.  

It would be helpful if the log entries were numbered - I know that this is not part of this bug but as I was testing it would have been helpful.  I can submit that as another bug.

Here's an example from testing - https://snipboard.io/ZFnmI0.jpg

Created attachment 108692 [details] [review]
Bug 26207: Compare values of system preference log entries

This patch adds the ability to compare the values of system preferences
in the system log. The user can select two entries in the log search
results and view a diff of the two versions.

The feature is designed specifically for CSS and JS preferences like
OpacUserCSS or OpacUserJS where code changes are difficult to track.

To test, apply the patch and go to Tools -> Log viewer. You may need to
add values and make changes to preferences like OpacUserCSS if you don't
already have a history in your logs.

- Perform a search for log entries in the "System prefs" module.
- In the results, each entry for a system preference should have a
  "Compare" checkbox."
- Check one of the checkboxes.
  - The table should now be filtered by the system preference name. This
    facilitates meaningful selections for comparison.
  - You should see a message displayed onscreen, "Showing results
    for...[ preference name]"
  - The "Check none" link in the toolbar above the table should now be
    enabled.
  - If youy click the "Compare selected" link in the toolbar at this
    stage you should get a message, "You must select two entries to
    compare."
  - If you uncheck the checkbox the table should return to an unfiltered
    state.
- Check two checkboxes.
  - The "Compare selected" link in the toolbar should now be enabled.
  - Next to each of the checkboxes you checked should be a "View
    comparison" link.
  - Clicking either of the "View comparison" links or the "Compare
    selected" link should trigger a modal with the diff view of the two
    preferences.
- Try to select a third checkbox. You should get a message, "You can
  select a maximum of two checkboxes." The box should remain unchecked.
- Test that unchecking both checkboxes manually clears the table filter.
- Test that the "Check none" link works to uncheck checked boxes and
  clear the table filter.

Signed-off-by: Brandon J <brandon.jimenez@inLibro.com>

(In reply to Barbara Johnson from comment #4)

> For libraries that have a lot of CSS or jQuery it would be helpful to add a
> horizontal scroll bar at the top of the modal.

I don't think this is something I can add. The scrollbars are generated by the browser based on the overflow of the content in each pane.
 
> I think it would be preferable to have the first line of the modal left
> justified - right now it is indented.


This is a whitespace issue which I can address in a follow-up.

> Also, it would be nice to have the name of the system preference (such as
> IntranetUserJS) on the first line of the modal with the first line of actual
> code displaying on a separate line

I think the whitespace correction will at least help.

Created attachment 108736 [details] [review]
Bug 26207: (follow-up) Remove whitespace from markup

Markup indentation around log entries in the template cause extra
whitespace in the diff view. Removing the markup whitespace corrects it.

Created attachment 108760 [details] [review]
Bug 26207: Compare values of system preference log entries

This patch adds the ability to compare the values of system preferences
in the system log. The user can select two entries in the log search
results and view a diff of the two versions.

The feature is designed specifically for CSS and JS preferences like
OpacUserCSS or OpacUserJS where code changes are difficult to track.

To test, apply the patch and go to Tools -> Log viewer. You may need to
add values and make changes to preferences like OpacUserCSS if you don't
already have a history in your logs.

- Perform a search for log entries in the "System prefs" module.
- In the results, each entry for a system preference should have a
  "Compare" checkbox."
- Check one of the checkboxes.
  - The table should now be filtered by the system preference name. This
    facilitates meaningful selections for comparison.
  - You should see a message displayed onscreen, "Showing results
    for...[ preference name]"
  - The "Check none" link in the toolbar above the table should now be
    enabled.
  - If youy click the "Compare selected" link in the toolbar at this
    stage you should get a message, "You must select two entries to
    compare."
  - If you uncheck the checkbox the table should return to an unfiltered
    state.
- Check two checkboxes.
  - The "Compare selected" link in the toolbar should now be enabled.
  - Next to each of the checkboxes you checked should be a "View
    comparison" link.
  - Clicking either of the "View comparison" links or the "Compare
    selected" link should trigger a modal with the diff view of the two
    preferences.
- Try to select a third checkbox. You should get a message, "You can
  select a maximum of two checkboxes." The box should remain unchecked.
- Test that unchecking both checkboxes manually clears the table filter.
- Test that the "Check none" link works to uncheck checked boxes and
  clear the table filter.

Signed-off-by: Brandon J <brandon.jimenez@inLibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108761 [details] [review]
Bug 26207: (follow-up) Remove whitespace from markup

Markup indentation around log entries in the template cause extra
whitespace in the diff view. Removing the markup whitespace corrects it.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

I really like this feature!

Only thing I was wondering is the label on the checkbox. Currently it reads 'compare' but it also has a filter function. I wonder if something like "Filter and compare" would work?

(In reply to Katrin Fischer from comment #10)
>  Currently it reads 'compare' but it also has a filter function.

I hoped that any confusion would be mitigated by the pop-up message that results were filtered after checking the box.

Some suggestions:
1. Isn't the popup superfluous? It appears quite often when you start clicking. Even when the second checkbox is unchecked.
2. "Check none" and "Compare selected" are styled differently when disabled, is that expected?
3. Should not we remove the toolbar if there is no syspref in the table result?
And/or have a on hover tooltip to tell what it is?
4. Isn't the blue background colour too visible? We could have it only for the label (not on all the line)?
https://snipboard.io/Rpdkxq.jpg vs https://snipboard.io/lvUfhV.jpg (I added a float: right)
5. Since the toolbar is float, aren't "Compare selected" and "View comparison" duplicate?
https://snipboard.io/NuXwMp.jpg

All for discussion, not considering blocker.

(In reply to Jonathan Druart from comment #12)

Item #4 - I actually prefer the blue background color extending the full width.  We have a lot of jQuery and CSS - having that color to look for helped me to find the bottom of the long entries where the compare and select buttons are.

Created attachment 110848 [details] [review]
Bug 26207: (follow-up) Hide comparison toolbar if there are no comparisons

This patch adds a check to the JavaScript so that the comparisons
toolbar will be hidden if there are no system preferences in the results
to compare.

(In reply to Jonathan Druart from comment #12)
> Some suggestions:
> 1. Isn't the popup superfluous? It appears quite often when you start
> clicking. Even when the second checkbox is unchecked.

It may be, but I think in this situation it's better to make it very clear to the user what is going on.

> 2. "Check none" and "Compare selected" are styled differently when disabled,
> is that expected?

Yes.

> 3. Should not we remove the toolbar if there is no syspref in the table
> result?

Addressed in the last followup.

> 5. Since the toolbar is float, aren't "Compare selected" and "View
> comparison" duplicate?

As with question #1, I think in this situation some redundancy is good for clarity.

Pushed to master for 20.11, thanks to everybody involved!

Owen, could we have this for reports log too?

enhancement will not be backported to 20.05.x