Bug 10933 - Merge PurgeSearchHistory into the C4::Search::History module
Summary: Merge PurgeSearchHistory into the C4::Search::History module
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 10807
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-23 11:33 UTC by Jonathan Druart
Modified: 2016-06-21 21:40 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this: 2013-10-30 00:00:00
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module (11.17 KB, patch)
2013-09-23 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module (11.45 KB, patch)
2013-10-18 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module (11.43 KB, patch)
2014-04-23 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module (11.34 KB, patch)
2014-05-07 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module (11.64 KB, patch)
2015-08-31 11:45 UTC, j.kylmala
Details | Diff | Splinter Review
Bug 10933: (follow-up) fix the previous patch to work with master (1.19 KB, patch)
2015-08-31 11:46 UTC, j.kylmala
Details | Diff | Splinter Review
[PASSED QA] Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module (11.71 KB, patch)
2015-10-22 20:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10933: (follow-up) fix the previous patch to work with master (1.25 KB, patch)
2015-10-22 20:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
ug 10933: (follow-up of the follow-up) fix the previous patch to work with master (1.08 KB, patch)
2015-10-27 16:06 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-23 11:33:52 UTC

    
Comment 1 Jonathan Druart 2013-09-23 11:48:14 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-09-23 21:52:06 UTC
Applying: Bug 10807: Add an authority search history for the OPAC
Using index info to reconstruct a base tree...
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
Falling back to patching base and 3-way merge...
Removing t/db_dependent/Auth_ParseSearchHistoryCookie.t
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Patch failed at 0001 Bug 10807: Add an authority search history for the OPAC
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 3 I'm just a bot 2013-09-23 21:52:22 UTC
This bug depends on bug10807 which is in status Signed Off but the patches for it do not apply cleanly
Comment 4 Jonathan Druart 2013-10-18 14:40:30 UTC Comment hidden (obsolete)
Comment 5 I'm just a bot 2013-10-30 07:30:58 UTC
Applying: Bug 10807: Add an authority search history for the OPAC
Using index info to reconstruct a base tree...
M	C4/Auth.pm
M	C4/Search.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	opac/opac-search-history.pl
Falling back to patching base and 3-way merge...
Removing t/db_dependent/Auth_ParseSearchHistoryCookie.t
Auto-merging opac/opac-search-history.pl
CONFLICT (content): Merge conflict in opac/opac-search-history.pl
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Search.pm
Auto-merging C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Patch failed at 0001 Bug 10807: Add an authority search history for the OPAC
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 6 I'm just a bot 2013-10-30 07:31:14 UTC
This bug depends on bug10807 which is in status Signed Off but the patches for it do not apply cleanly
Comment 7 Jonathan Druart 2013-10-30 08:53:23 UTC
apply cleanly, patches for bug 10807 have been rebased.
Comment 8 Owen Leonard 2014-04-22 19:06:20 UTC
Sorry, this no longer cleanly applies on top of Bug 10807.
Comment 9 Jonathan Druart 2014-04-23 08:05:54 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2014-05-06 16:50:03 UTC
Patch doesn't apply.

Of more note, the change to C4::Search::History::add seems unrelated and dangerous, as no current callers of C4::Search::History:add seem to be passing a time parameter.
Comment 11 Jonathan Druart 2014-05-07 14:37:38 UTC
(In reply to Galen Charlton from comment #10)
> Patch doesn't apply.

It is only a unit file which was already deleted.

> Of more note, the change to C4::Search::History::add seems unrelated and
> dangerous, as no current callers of C4::Search::History:add seem to be
> passing a time parameter.

I don't understand why it is dangerous. Actually the search_history.time field has a default value in DB. I need to pass some hardcoded time (further back in time) for unit tests.
Comment 12 Jonathan Druart 2014-05-07 14:38:12 UTC Comment hidden (obsolete)
Comment 13 j.kylmala 2015-08-31 11:45:22 UTC Comment hidden (obsolete)
Comment 14 j.kylmala 2015-08-31 11:46:06 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2015-10-22 20:34:03 UTC
Hi Jonathan, can you take a quick look at the failing test please?

not ok 16 - Calling delete without userid raises warning
#   Failed test 'Calling delete without userid raises warning'
#   at t/db_dependent/Search/History.t line 128.
# found warning: ERROR: userid, id or interval is required for history deletion at /home/katrin/kohaclone/C4/Search/History.pm line 77.
# expected to find warning: (?^u:^ERROR: userid or id is required for history deletion)
Comment 16 Katrin Fischer 2015-10-22 20:35:38 UTC
Never mind - I fixed it, continuing with testing.
Comment 17 Katrin Fischer 2015-10-22 20:56:34 UTC
Created attachment 43786 [details] [review]
[PASSED QA] Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module

Since bug 10803 adds a C4::Search::History module, the
PurgeSearchHistory routine should be moved.

Test plan:
- run misc/cronjobs/cleanup_database.pl with the searchhistory param and
verify behavior is the same as before applying this patch.
- run prove t/Search/History.t

Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2015-10-22 20:56:49 UTC
Created attachment 43787 [details] [review]
[PASSED QA] Bug 10933: (follow-up) fix the previous patch to work with master

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Tomás Cohen Arazi 2015-10-27 14:08:07 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 20 Jonathan Druart 2015-10-27 16:06:02 UTC
Created attachment 44095 [details] [review]
ug 10933: (follow-up of the follow-up) fix the previous patch to work with master