Bug 38299

Summary: Errors with updates caught in C4::Installer should be colored/highlighted
Product: Koha Reporter: Martin Renvoize (ashimema) <martin.renvoize>
Component: Installation and upgrade (command-line installer)Assignee: Emily Lamancusa (emlam) <emily.lamancusa>
Status: RESOLVED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: david, emily.lamancusa, jonathan.druart, lucas, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38385
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:
Bug Depends on: 25078, 35681    
Bug Blocks:    
Attachments: Bug 38299: [DO NOT PUSH] Test dbrev
Bug 38299: Make filehandler optional for colored outputs
Bug 38299: Use say_failure when printing update errors to the console
Bug 38299: [DO NOT PUSH] Test dbrev
Bug 38299: Make filehandler optional for colored outputs
Bug 38299: Use say_failure when printing update errors to the console
Bug 38299: Update tests
Bug 38299: [DO NOT PUSH] Test dbrev
Bug 38299: Make filehandler optional for colored outputs
Bug 38299: Use say_failure when printing update errors to the console
Bug 38299: Update tests
Bug 38299: (discussion) Move failure into same level
Bug 38299: (discussion) Move failure into same level
With proper indentation and class
Bug 38299: (follow-up) (discussion) Move failure into same level
Bug 38299: Make filehandler optional for colored outputs
Bug 38299: Use say_failure when printing update errors to the console
Bug 38299: Update tests
Bug 38299: (discussion) Move failure into same level
Bug 38299: (follow-up) (discussion) Move failure into same level

Description Martin Renvoize (ashimema) 2024-10-30 14:45:55 UTC
We introduced color into our db update output, but we never implemented it in the upstream code that catches database level failures.
Comment 1 Emily Lamancusa (emlam) 2024-11-07 15:35:57 UTC
Created attachment 174250 [details] [review]
Bug 38299: [DO NOT PUSH] Test dbrev
Comment 2 Emily Lamancusa (emlam) 2024-11-07 15:35:59 UTC
Created attachment 174251 [details] [review]
Bug 38299: Make filehandler optional for colored outputs

Allow say_success, say_failure, etc, to omit the filehandler parameter
so that they can be used without explicitly opening a filehandler if you
are just printing to STDOUT and don't already have one.

If a filehandler is passed, the existing behavior is unchanged.
Comment 3 Emily Lamancusa (emlam) 2024-11-07 15:36:00 UTC
Created attachment 174252 [details] [review]
Bug 38299: Use say_failure when printing update errors to the console

Bug 35681 added subroutines to print colored output from database
updates. Incorporate the subroutine say_failure into the error handling
in the upstream code, so that errors caught upstream can take advantage
of this new subroutine and display in red.

To test:
1. Apply patch for test dbrev
2. From the kshell, run updatedatabase
--> The messages for success, info, warning, and failure that were
    explicitly printed in the dbrev are colored, but the database error
    is not colored.
3. Apply other patches
4. Run updatedatabase again
--> All messages including the database error are now colored
Comment 4 Emily Lamancusa (emlam) 2024-11-07 15:52:12 UTC
Created attachment 174254 [details] [review]
Bug 38299: [DO NOT PUSH] Test dbrev
Comment 5 Emily Lamancusa (emlam) 2024-11-07 15:52:14 UTC
Created attachment 174255 [details] [review]
Bug 38299: Make filehandler optional for colored outputs

Allow say_success, say_failure, etc, to omit the filehandler parameter
so that they can be used without explicitly opening a filehandler if you
are just printing to STDOUT and don't already have one.

If a filehandler is passed, the existing behavior is unchanged.
Comment 6 Emily Lamancusa (emlam) 2024-11-07 15:52:15 UTC
Created attachment 174256 [details] [review]
Bug 38299: Use say_failure when printing update errors to the console

Bug 35681 added subroutines to print colored output from database
updates. Incorporate the subroutine say_failure into the error handling
in the upstream code, so that errors caught upstream can take advantage
of this new subroutine and display in red.

To test:
1. Apply patch for test dbrev
2. From the kshell, run updatedatabase
--> The messages for success, info, warning, and failure that were
    explicitly printed in the dbrev are colored, but the database error
    is not colored.
3. Apply other patches
4. Run updatedatabase again
--> All messages including the database error are now colored
Comment 7 Emily Lamancusa (emlam) 2024-11-07 16:22:01 UTC
Created attachment 174259 [details] [review]
Bug 38299: Update tests

To test:
prove -v t/Koha/Installer/Output.t
Comment 8 David Nind 2024-11-08 19:29:30 UTC
Created attachment 174290 [details] [review]
Bug 38299: [DO NOT PUSH] Test dbrev

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2024-11-08 19:29:33 UTC
Created attachment 174291 [details] [review]
Bug 38299: Make filehandler optional for colored outputs

Allow say_success, say_failure, etc, to omit the filehandler parameter
so that they can be used without explicitly opening a filehandler if you
are just printing to STDOUT and don't already have one.

If a filehandler is passed, the existing behavior is unchanged.

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2024-11-08 19:29:36 UTC
Created attachment 174292 [details] [review]
Bug 38299: Use say_failure when printing update errors to the console

Bug 35681 added subroutines to print colored output from database
updates. Incorporate the subroutine say_failure into the error handling
in the upstream code, so that errors caught upstream can take advantage
of this new subroutine and display in red.

To test:
1. Apply patch for test dbrev
2. From the kshell, run updatedatabase
--> The messages for success, info, warning, and failure that were
    explicitly printed in the dbrev are colored, but the database error
    is not colored.
3. Apply other patches
4. Run updatedatabase again
--> All messages including the database error are now colored

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2024-11-08 19:29:38 UTC
Created attachment 174293 [details] [review]
Bug 38299: Update tests

To test:
prove -v t/Koha/Installer/Output.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Martin Renvoize (ashimema) 2024-11-12 15:26:32 UTC
Created attachment 174421 [details] [review]
Bug 38299: (discussion) Move failure into same level

This patch adds the failures in at the same level as the rest of the
revision output (i.e. indented under the specific database revision) and
uses the standard failure coloring instead of the local update_error
class.

This is a proposal and may need more work to clarify and perhaps remove
the duplicated/collected errors at the bottom of the update. Now that we
hard stop if an update fails, I'm not sure collecting errors at the
bottom makes as much sense as it once did?
Comment 13 Martin Renvoize (ashimema) 2024-11-12 15:44:53 UTC
Created attachment 174423 [details] [review]
Bug 38299: (discussion) Move failure into same level

This patch adds the failures in at the same level as the rest of the
revision output (i.e. indented under the specific database revision) and
uses the standard failure coloring instead of the local update_error
class.

This is a proposal and may need more work to clarify and perhaps remove
the duplicated/collected errors at the bottom of the update. Now that we
hard stop if an update fails, I'm not sure collecting errors at the
bottom makes as much sense as it once did?
Comment 14 Martin Renvoize (ashimema) 2024-11-12 15:45:36 UTC
Created attachment 174424 [details]
With proper indentation and class
Comment 15 Martin Renvoize (ashimema) 2024-11-12 15:48:07 UTC
It feels somewhat odd looking to me that we collect some errors separately at the end.. I've added a patch that moves the output up a level into the output block for database revision being run..  I've left the output to stderr in place for now so we're getting both the collected set and the inlined error in the screenshot.  Wondering what others think here.. now that we die on error I'm thinking it no longer makes sense to collect such errors separately at the bottom and we could just remove that bit of handling.
Comment 16 Jonathan Druart 2024-11-13 11:15:13 UTC
Created attachment 174455 [details] [review]
Bug 38299: (follow-up) (discussion) Move failure into same level
Comment 17 Jonathan Druart 2024-11-13 11:15:23 UTC
(In reply to Martin Renvoize (ashimema) from comment #13)
> Created attachment 174423 [details] [review] [review]
> Bug 38299: (discussion) Move failure into same level
> 
> This patch adds the failures in at the same level as the rest of the
> revision output (i.e. indented under the specific database revision) and
> uses the standard failure coloring instead of the local update_error
> class.
> 
> This is a proposal and may need more work to clarify and perhaps remove
> the duplicated/collected errors at the bottom of the update. Now that we
> hard stop if an update fails, I'm not sure collecting errors at the
> bottom makes as much sense as it once did?

Looks ok to me, Emily can you confirm you agree with this change?
Comment 18 Jonathan Druart 2024-11-13 11:16:38 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2024-11-13 11:16:54 UTC
I think I might have preferred to have the fh as an 'out' option, like we do for 'up', but not blocker.
Comment 20 Emily Lamancusa (emlam) 2024-11-13 17:56:14 UTC
Looks good to me! Thanks for the feedback on this!
Comment 21 Jonathan Druart 2024-11-18 10:31:54 UTC
Created attachment 174673 [details] [review]
Bug 38299: Make filehandler optional for colored outputs

Allow say_success, say_failure, etc, to omit the filehandler parameter
so that they can be used without explicitly opening a filehandler if you
are just printing to STDOUT and don't already have one.

If a filehandler is passed, the existing behavior is unchanged.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2024-11-18 10:31:57 UTC
Created attachment 174674 [details] [review]
Bug 38299: Use say_failure when printing update errors to the console

Bug 35681 added subroutines to print colored output from database
updates. Incorporate the subroutine say_failure into the error handling
in the upstream code, so that errors caught upstream can take advantage
of this new subroutine and display in red.

To test:
1. Apply patch for test dbrev
2. From the kshell, run updatedatabase
--> The messages for success, info, warning, and failure that were
    explicitly printed in the dbrev are colored, but the database error
    is not colored.
3. Apply other patches
4. Run updatedatabase again
--> All messages including the database error are now colored

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2024-11-18 10:31:59 UTC
Created attachment 174675 [details] [review]
Bug 38299: Update tests

To test:
prove -v t/Koha/Installer/Output.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2024-11-18 10:32:02 UTC
Created attachment 174676 [details] [review]
Bug 38299: (discussion) Move failure into same level

This patch adds the failures in at the same level as the rest of the
revision output (i.e. indented under the specific database revision) and
uses the standard failure coloring instead of the local update_error
class.

This is a proposal and may need more work to clarify and perhaps remove
the duplicated/collected errors at the bottom of the update. Now that we
hard stop if an update fails, I'm not sure collecting errors at the
bottom makes as much sense as it once did?

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2024-11-18 10:32:05 UTC
Created attachment 174677 [details] [review]
Bug 38299: (follow-up) (discussion) Move failure into same level

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Katrin Fischer 2024-11-18 16:45:52 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 27 Katrin Fischer 2024-11-19 08:10:05 UTC
It looks like we caused some bad breakage on Jenkins with this one, because I pushed the "DO NOT PUSH". Will revert.
Comment 28 Lucas Gass (lukeg) 2024-12-05 23:01:29 UTC
Merge conflicts with 24.05.x, no backport.
Comment 29 David Nind 2024-12-16 20:55:26 UTC
No updates to the manual required - it is not covered.

I'll leave any updates required to the Developer Handbook, to the patch author.
Comment 30 Emily Lamancusa (emlam) 2024-12-16 21:02:54 UTC
Thanks, David! No developer documentation updates are required either - these patches don't require any changes to how database updates are coded.
Comment 31 David Nind 2024-12-16 21:11:44 UTC
(In reply to Emily Lamancusa (emlam) from comment #30)
> Thanks, David! No developer documentation updates are required either -
> these patches don't require any changes to how database updates are coded.

Thanks Emily!