Bug 26207 - Compare values of system preference log entries
Summary: Compare values of system preference log entries
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 26736
  Show dependency treegraph
 
Reported: 2020-08-13 18:43 UTC by Owen Leonard
Modified: 2020-11-12 12:53 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This patch adds a feature to the log viewer in the staff interface for use when viewing system preference log entries. The feature allows the user to select two system preference values in the log for comparison. The two versions are shown in a modal window with the differences highlighted.
Version(s) released in:
20.11.00


Attachments
Bug 26207: Compare values of system preference log entries (12.16 KB, patch)
2020-08-14 12:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26207: Compare values of system preference log entries (12.20 KB, patch)
2020-08-19 19:33 UTC, Brandon J
Details | Diff | Splinter Review
Bug 26207: (follow-up) Remove whitespace from markup (2.13 KB, patch)
2020-08-20 12:51 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26207: Compare values of system preference log entries (12.27 KB, patch)
2020-08-20 22:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26207: (follow-up) Remove whitespace from markup (2.18 KB, patch)
2020-08-20 22:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26207: (follow-up) Hide comparison toolbar if there are no comparisons (1.09 KB, patch)
2020-09-28 11:19 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2020-08-13 18:43:43 UTC
I could be useful to offer a diff tool for system preference update entries in the log viewer. This is especially relevant to preferences like OpacUserCSS or OpacUserJS where it might be difficult to spot what changes were made.
Comment 1 Owen Leonard 2020-08-13 18:44:57 UTC
*It* could be useful. But yes, I could be useful too I guess.
Comment 2 Lucas Gass 2020-08-13 20:34:15 UTC
+1
Comment 3 Owen Leonard 2020-08-14 12:48:58 UTC
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.
Comment 4 Barbara Johnson 2020-08-14 15:39:11 UTC
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
Comment 5 Brandon J 2020-08-19 19:33:26 UTC
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>
Comment 6 Owen Leonard 2020-08-20 12:46:47 UTC
(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.
Comment 7 Owen Leonard 2020-08-20 12:51:44 UTC
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.
Comment 8 Katrin Fischer 2020-08-20 22:07:46 UTC
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>
Comment 9 Katrin Fischer 2020-08-20 22:07:49 UTC
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>
Comment 10 Katrin Fischer 2020-08-20 22:09:43 UTC
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?
Comment 11 Owen Leonard 2020-08-21 15:06:36 UTC
(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.
Comment 12 Jonathan Druart 2020-08-24 09:18:47 UTC
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.
Comment 13 Barbara Johnson 2020-08-31 16:11:53 UTC
(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.
Comment 14 Owen Leonard 2020-09-28 11:19:01 UTC
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.
Comment 15 Owen Leonard 2020-09-28 11:24:22 UTC
(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.
Comment 16 Jonathan Druart 2020-10-01 09:45:01 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Katrin Fischer 2020-10-17 16:45:19 UTC
Owen, could we have this for reports log too?
Comment 18 Lucas Gass 2020-10-20 17:39:03 UTC
enhancement will not be backported to 20.05.x