|
Description
Andrew Fuerste-Henry
2019-04-03 19:32:01 UTC
Created attachment 87778 [details] [review] without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 87920 [details] [review] Bug 21003: Made Edit keywords more consistent in add_items-step2.tt When adding a course reserve with a barcode that already exists, the title, breadcrumbs, and legend of the form all specify that the user is editing, and not adding, the book. Test plan: Same as comment 1 Signed-off-by: Devinim <kohadevinim@devinim.com.tr> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> I think something went wrong here with the patch attached. Obsoleting the last one. Created attachment 87921 [details] [review] Unit Testing of patch 22632 Created attachment 87922 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 87923 [details] [review] Unit Testing of patch 22632 Created attachment 87926 [details] [review] adding of the rescearch of merge in the log viewer Created attachment 87995 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 87996 [details] [review] Bug 22632: Unit Testing of patch 22632 Created attachment 87997 [details] [review] Bug 22632: adding of the rescearch of merge in the log viewer Created attachment 87998 [details] [review] Bug 22632: Change for the research of Modules : Patrons Created attachment 87999 [details] [review] Bug 22632: Change for the research of Modules : Patrons Created attachment 88043 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 88044 [details] [review] Bug 22632: Unit Testing of patch 22632 Created attachment 88045 [details] [review] Bug 22632: adding of the rescearch of merge in the log viewer Created attachment 88046 [details] [review] Bug 22632: Change for the research of Modules : Patrons Would it be possible for this to get entered in the log with the retained patron as the object, rather than the deleted patron? That way it would show up in the modification log for the extant patron, which seems a bit more helpful than having it tie to the deleted patron. Other than that small nitpick, this is great! (In reply to Andrew from comment #17) > Would it be possible for this to get entered in the log with the retained > patron as the object, rather than the deleted patron? That way it would show > up in the modification log for the extant patron, which seems a bit more > helpful than having it tie to the deleted patron. > > Other than that small nitpick, this is great! hi, in info we have : patron 1 has been been merged into patron 2, so we see both patrons. Did you mean to reverse the sentence in: patron 2 has been merged with patron 1 ? (In reply to axel Amghar from comment #18) > (In reply to Andrew from comment #17) > > Would it be possible for this to get entered in the log with the retained > > patron as the object, rather than the deleted patron? That way it would show > > up in the modification log for the extant patron, which seems a bit more > > helpful than having it tie to the deleted patron. > > > > Other than that small nitpick, this is great! > > hi, > > in info we have : > patron 1 has been been merged into patron 2, so we see both patrons. > > Did you mean to reverse the sentence in: > patron 2 has been merged with patron 1 ? Hi Axel, there is an object column in the action_log table - I think he refers to that. Not to the info part. Hi folks, Right, as Katrin says, I'm looking at the Object column. I realize both patrons are in the Info column, so this works ok. But as it is, if I merge patrons 1 & 2, retaining patron 1, and then click Modification Log from patron 1's record the merge doesn't appear in the log search Koha generates. What Andrew suggests makes sense to me! Created attachment 89728 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 89729 [details] [review] Bug 22632: Unit Testing of patch 22632 Created attachment 89730 [details] [review] Bug 22632: adding of the rescearch of merge in the log viewer (In reply to Katrin Fischer from comment #21) > What Andrew suggests makes sense to me! Hi, thank you for for the explanations :) I think i get it, tell me if it's better. i have issues with the unit test so i put the status in assigned Created attachment 89731 [details] [review] Bug 22632: Unit Testing of patch 22632 Current status: Needs Signoff Patch doesn't apply. The 'Merge' is not recorded in the log viewer, only the deleted account is shown. Hi This has come up as an issue with Cumbria Libraries and the ability to see the previous information on a merged patron record in the modification logs would be really useful. I am told that the information is retained in the deleted borrowers table however that would require a report to access the information and our frontline staff would find it very useful to be able to see these deleted personal details from the merged account. We get alot of duplicate memberships from patrons re-joining online, sometimes with more uptodate information than their existing record has and it would be useful for staff to have this information if a patron queries the merge. Created attachment 157260 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 157261 [details] [review] Bug 22632: adding of the rescearch of merge in the log viewer Created attachment 157262 [details] [review] Bug 22632: Unit Testing of patch 22632 Created attachment 157263 [details] [review] Bug 22632: (follow-up) Use PATRON_MERGE as log action Created attachment 157264 [details] [review] Bug 22632: (follow-up) Perltidy fixes for Koha/Patron.pm Created attachment 157265 [details] [review] Bug 22632: (follow-up) Apply perltidy fixes and add missing exec flag to Log.t I've rebased this and added some cleanup follow-ups, but I think this still needs work to address all the QA tool issues. Hopefully someone else can take this to the finish line? Created attachment 157266 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Signed-off-by: David Nind <david@davidnind.com> Created attachment 157267 [details] [review] Bug 22632: adding of the rescearch of merge in the log viewer Signed-off-by: David Nind <david@davidnind.com> Created attachment 157268 [details] [review] Bug 22632: Unit Testing of patch 22632 Signed-off-by: David Nind <david@davidnind.com> Created attachment 157269 [details] [review] Bug 22632: (follow-up) Use PATRON_MERGE as log action Signed-off-by: David Nind <david@davidnind.com> Created attachment 157270 [details] [review] Bug 22632: (follow-up) Perltidy fixes for Koha/Patron.pm Signed-off-by: David Nind <david@davidnind.com> Created attachment 157271 [details] [review] Bug 22632: (follow-up) Apply perltidy fixes and add missing exec flag to Log.t Signed-off-by: David Nind <david@davidnind.com> I signed off before seeing comment #36 What should the status be changed to? Failed QA? Also now noted that the tests fail: prove t/db_dependent/Koha/Patron/Log.t t/db_dependent/Koha/Patron/Log.t .. 1/1 # No tests run! # Failed test 'No tests run for subtest "Test Koha::Patrons::merge"' # at t/db_dependent/Koha/Patron/Log.t line 97. Undefined subroutine &main::GetLogs called at t/db_dependent/Koha/Patron/Log.t line 44. # Looks like your test exited with 11 just after 1. t/db_dependent/Koha/Patron/Log.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 1/1 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patron/Log.t (Wstat: 2816 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 11 Files=1, Tests=1, 1 wallclock secs ( 0.02 usr 0.00 sys + 1.11 cusr 0.26 csys = 1.39 CPU) Result: FAIL Created attachment 157278 [details] [review] Bug 22632: (follow-up) Tidy and fix tests (In reply to David Nind from comment #43) > I signed off before seeing comment #36 > > What should the status be changed to? Failed QA? > Tests should pass now, and tidied - can you confirm tests and sign off David? (In reply to Nick Clemens from comment #45) > (In reply to David Nind from comment #43) > > I signed off before seeing comment #36 > > > > What should the status be changed to? Failed QA? > > > Tests should pass now, and tidied - can you confirm tests and sign off David? I'm getting a sha1 fake ancestor error when trying to apply the patches now. git bz apply 22632 Bug 22632 - Add logging of merged patrons 157266 - Bug 22632: Add logging of merged patrons 157267 - Bug 22632: adding of the rescearch of merge in the log viewer 157268 - Bug 22632: Unit Testing of patch 22632 157269 - Bug 22632: (follow-up) Use PATRON_MERGE as log action 157270 - Bug 22632: (follow-up) Perltidy fixes for Koha/Patron.pm 157271 - Bug 22632: (follow-up) Apply perltidy fixes and add missing exec flag to Log.t 157278 - Bug 22632: (follow-up) Tidy and fix tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 22632: Add logging of merged patrons Applying: Bug 22632: adding of the rescearch of merge in the log viewer Applying: Bug 22632: Unit Testing of patch 22632 Applying: Bug 22632: (follow-up) Use PATRON_MERGE as log action Applying: Bug 22632: (follow-up) Perltidy fixes for Koha/Patron.pm Applying: Bug 22632: (follow-up) Apply perltidy fixes and add missing exec flag to Log.t Applying: Bug 22632: (follow-up) Tidy and fix tests error: sha1 information is lacking or useless (Koha/Patron.pm). error: could not build fake ancestor Patch failed at 0001 Bug 22632: (follow-up) Tidy and fix tests hint: Use 'git am --show-current-patch=diff' to see the failed patch ..... Created attachment 157281 [details] [review] Bug 22632: (follow-up) Tidy and fix tests Created attachment 157286 [details] [review] Bug 22632: (follow-up) Tidy and fix tests Signed-off-by: David Nind <david@davidnind.com> Thanks Nick! Tests now pass, signed off. Can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 22632: Add logging of merged patrons Applying: Bug 22632: adding of the rescearch of merge in the log viewer Applying: Bug 22632: Unit Testing of patch 22632 Applying: Bug 22632: (follow-up) Use PATRON_MERGE as log action Applying: Bug 22632: (follow-up) Perltidy fixes for Koha/Patron.pm Applying: Bug 22632: (follow-up) Apply perltidy fixes and add missing exec flag to Log.t Applying: Bug 22632: (follow-up) Tidy and fix tests error: sha1 information is lacking or useless (Koha/Patron.pm). error: could not build fake ancestor Patch failed at 0001 Bug 22632: (follow-up) Tidy and fix tests hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-22632-follow-up-Tidy-and-fix-tests-o_fvd6hr.patch Created attachment 159553 [details] [review] Bug 22632: Add logging of merged patrons without this patch , when patrons are merged , you can't see the merge in the log viewer. To test: -Apply the patch -Make sure you have "BorrowersLog" on in you System Preference -Search patrons -Check two ore more patrons -Merge selected patrons -Merge patrons -Go to log viewer - Make sure there is the line with the action : "Merge" Created attachment 159554 [details] [review] Bug 22632: adding of the rescearch of merge in the log viewer Created attachment 159555 [details] [review] Bug 22632: Unit Testing of patch 22632 Created attachment 159556 [details] [review] Bug 22632: (follow-up) Use PATRON_MERGE as log action Created attachment 159557 [details] [review] Bug 22632: (follow-up) Perltidy fixes for Koha/Patron.pm Created attachment 159558 [details] [review] Bug 22632: (follow-up) Apply perltidy fixes and add missing exec flag to Log.t Your tests include a deleted subroutine, C4::Logs::GetLogs. You must now use Koha::ActionLogs->search instead. |