|
Description
David Bourgault
2017-09-26 17:47:25 UTC
Created attachment 67373 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Comment on attachment 67373 [details] [review] Bug 19368 - Ignore words when sorting serial search Review of attachment 67373 [details] [review]: ----------------------------------------------------------------- Nice enhancement David. Just few comments/issues, all looks easy to fix ;) In update database file, don't use INSERT IGNORE, it is mysqlism. Better to make update files using perl skeleton and do needed db tests in the code. ::: installer/data/mysql/sysprefs.sql @@ +475,4 @@ > ('SelfCheckTimeout','120','','Define the number of seconds before the Web-based Self Checkout times out a patron','Integer'), > ('SeparateHoldings','0',NULL,'Separate current branch holdings from other holdings','YesNo'), > ('SeparateHoldingsBranch','homebranch','homebranch|holdingbranch','Branch used to separate holdings','Choice'), > +('SerialSortIgnoreWords','des du de la le les leur d l an a the',NULL,'Words to ignore in serial search','Free'), Your commit message says, it's empty by default... which is good idea IMHO ::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt @@ +38,4 @@ > return confirm(_("Are you sure you want to reopen this subscription?")); > }); > }); > + $ typo? ::: serials/serials-search.pl @@ +139,5 @@ > + } > + $subsctitle =~ s/^\s+//; > + $sub->{titlesort} = ucfirst($subsctitle); > + > + warn $subsctitle; please, remove this warn Created attachment 67447 [details] [review] Bug 19368 - Correction on QA feedback - Removed 'IGNORE' in the atomicupdate SQL. There is currently no other systempreference by that name in master so that should work fine - Removed leftover test values from syspref.sql - Removed extra '$' in serial-search.tt - Removed leftover 'warn' in serial-search.pl Created attachment 67580 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 67581 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> I feel like this is trying to achieve something for one table, that we need a more general solution for. At the moment we already do something like this for other tables using a translatable string: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/intranet-tmpl/prog/en/includes/datatables.inc;h=72be3f10e834e9b478e1e66bbb60c3a9c39c91d4;hb=a1bf319829c9d44991daa2fcf2593420f8445c5b var CONFIG_EXCLUDE_ARTICLES_FROM_SORT = _("a an the"); Maybe we should just move this string to be a system preference and then use it for serials too? I think with the patch both methods would be used at the same time - anti-the and the new perl based method. Note: Example content of the pref should at least also contain English articles! Created attachment 67612 [details] [review] Bug 19368 - Syspref now "SortIgnoreArticles", used in datatables.inc I updated the systempreference name to something more global (SortIgnoreWords) and updated datatables.inc to use it (opac and intranet). Test plan is same as before, plus testing any table that uses datatables.inc. Sponsored by : CCSR Hi David The syspref SortIgnoreArticles is not being displaying in the Global System preferences module to fix this can you please add SortIgnoreArticles syspref to the admin.pref YAML file Created attachment 69005 [details] [review] Bug 19368 - Syspref now "SortIgnoreArticles", used in datatables.inc Updated incorrect YAML preference file (Reply to David Bourgault in comment 12) Yes I would have thought the serials.pref file would be the most suitable YAML file to be updated. I think I was wrong in comment 10. Can you please add a patch to add SortIgnoreArticles to serials.pref As this now not only works for Serials, but for datatables in general, I think maybe it would better fit on the 'staff client' tab? Hope I didn't misunderstand the change. Created attachment 70051 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 70052 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 70053 [details] [review] Bug 19368 - Syspref now "SortIgnoreArticles", used in datatables.inc Necessary rebase Created attachment 70125 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref (In reply to Katrin Fischer from comment #14) > As this now not only works for Serials, but for datatables in general, I > think maybe it would better fit on the 'staff client' tab? Hope I didn't > misunderstand the change. The preference was recently moved from serials.pref to searching.pref to account, as you said, for the more 'global' impact the change has on datatables. I have no qualms moving it though, if admin/preferences/searching.pref is not where you feel it should be. Created attachment 71962 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 71963 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 71964 [details] [review] Bug 19368 - Syspref now "SortIgnoreArticles", used in datatables.inc Created attachment 71965 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Rebased on current master. Comment on attachment 71962 [details] [review] Bug 19368 - Ignore words when sorting serial search You're comparing the stopwords to the _unaccented_ subscription title. This isn't mentioned in the SerialSortIgnoreWords description. Created attachment 72043 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 72044 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 72045 [details] [review] Bug 19368 - Syspref now "SortIgnoreArticles", used in datatables.inc Created attachment 72046 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Created attachment 72047 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title As per comment#26, I made sure to warn the end-user, in the pref's description, that the comparison was done against the unaccented title. Rebased the rest on the current master. Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 72655 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 72656 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 72657 [details] [review] Bug 19368 - Syspref now "SortIgnoreArticles", used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 72658 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 72659 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> This patch works perfectly ! A detail : the syspref is "SortIgnoreArticles" and not "SerialSortIgnoreWords". Thanks for this work ! Created attachment 74262 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 74263 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 74264 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 74265 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 74266 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> New rebase. For some reason, patch couldn't be applied on master and failed with the useful "error: could not build fake ancestor". I like this idea, but I think I am confused here.
The pref now affects all datatables where we use anti-the, however, we still generate the extra column in serials-search and sort against that initially?
This code also seems to automatically strip articles before apostrophes - this works for some languages, but would it work for all? I don't know if the 'anti-the' plugin works with apostrophe'd articles, can you weigh in on that severine or someone else?
I find it confusing that the table is initially sorted by one column, then uses another if you click. Adding this to datatables seems to allow sorting by a hidden column:
{ 'orderData': 2, 'targets': 0},
Hi Nick ! I'm not sure I understood all your comments, but I've tested once more and here are my feedbacks. (In reply to Nick Clemens from comment #46) > The pref now affects all datatables where we use anti-the, When you say 'all databases using anti-the', can you tell me which one are concernd ? Do you have some ideas it could impact staff client and so where I should test ? I've tried on the /cgi-bin/koha/circ/circulation.pl page for the issues-table : articles are ignored even when the syspref is empty. > however, we still generate the extra column in serials-search and sort against that initially? I don't understand that part. > This code also seems to automatically strip articles before apostrophes - > this works for some languages, but would it work for all? I don't know if > the 'anti-the' plugin works with apostrophe'd articles, can you weigh in on > that severine or someone else? I tested with " L' " and " D' ", used very often in french and they are correctly ignore for sorting if I save them on the syspref. Do you have ideas of what of strings / articles / apostrophes use could be a problem ? > I find it confusing that the table is initially sorted by one column, then > uses another if you click. Adding this to datatables seems to allow sorting > by a hidden column: > { 'orderData': 2, 'targets': 0}, I don't understand that part either... Created attachment 90915 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 90916 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 90918 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 90919 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 90920 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 90921 [details] [review] Bug 19368 - Added missing closing bracket at the end of serials-search.pl Rebased on current master and added missing bracket after resolving conflicts. Sorry Nadine I didn't try sooner, the patch doesn’t apply on more time. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 19368 - Ignore words when sorting serial search Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/serials.pref M koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/serials.pref CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/serials.pref Auto-merging installer/data/mysql/sysprefs.sql Failed to merge in the changes. Patch failed at 0001 Bug 19368 - Ignore words when sorting serial search Created attachment 113018 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113019 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113020 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113021 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113022 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113023 [details] [review] Bug 19368 - Added missing closing bracket at the end of serials-search.pl Now I test it one more time, I noticed a few things...
Unexpected characters like '' can't be ignored (we've got plenty of those in our titles because of some ugly imported datas...)
I now most of devs won't agree with me, but I think it could be great if this feature finally doesn't ignore accents. For example, the word 'Thé' in French means tea and if I have a subscription beginning with it ('Thé ou café ?' for instance), I don't want it to be ignored.
The same for A and À.
I also have a question about sorting a title like 'À l'abordage !' or 'À l'ombre de mon arbre' : if i set the syspref to ignore both à and l' , should the 2 articles be ignored or only the first one ? For now, only the first is ignores.
Except these 2 points, this patch is great and works as expected !
Frankly, the accent part is arguable. The main reason it's like that is because comparing accents is a nightmare. They look the same, they sound the same, they ARE the same, except they are coded differently (UTF8 and friends) so the comparison fails and the patch is mosly useless in French and other accentuated languages. As for the last question, there's (again) no perfect answer. Right now it'd skip the À and keep the L'. This is one of those patches that we'd like to go through because it offers an improvement on the current code for most users (and usages) in its current form. Trying to solve the details will put it in In Discussion purgatory. Created attachment 113344 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113345 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113346 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113347 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113348 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 113349 [details] [review] Bug 19368 - Added missing closing bracket at the end of serials-search.pl Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> I completely understand, I expected this kind of answer. Just pointing some complementary points. This patch behaves as it's expected and the information about accent is present in the syspref, so I don't see now reasons for not signing it :) A last detail : syspref is now named 'SortIgnoreArticles' and not 'SerialSortIgnoreWords' as in the test plan. Applying: Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc error: sha1 information is lacking or useless (installer/data/mysql/mandatory/sysprefs.sql). Created attachment 118963 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Created attachment 118964 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Created attachment 118965 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Created attachment 118966 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Created attachment 118967 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Created attachment 118968 [details] [review] Bug 19368 - Added missing closing bracket at the end of serials-search.pl - Are the changes to datatables.inc necessary? I think this has made the one change you want (CONFIG_EXCLUDE_ARTICLES_FROM_SORT) obsolete: https://git.koha-community.org/Koha-community/Koha/commit/0c9b39a66dff00d2d76effc42824c2051484f9c5 - Shouldn't the preference description omit the reference to serial search results since the its use is theoretically general? - Is it necessary to replace [% INCLUDE subscriptions_table %] in serials-search.tt? Going back to all that markup after switching to an include seems like a step backwards. - Don't forget to run the QA tool to check for other issues after making corrections. Created attachment 124090 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SortIgnoreArticles pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Created attachment 124091 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124092 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124093 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124094 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124095 [details] [review] Bug 19368 - Added missing closing bracket at the end of serials-search.pl Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124096 [details] [review] Bug 19368 - (QA follow-up) Correction on QA feedback Created attachment 124097 [details] [review] Bug 19368 - Deleting useless variables in datatables.inc I tried to understand where it was used but I finally came to the conclusion that this is useless. (In reply to Owen Leonard from comment #78) > - Are the changes to datatables.inc necessary? I think this has made the one > change you want (CONFIG_EXCLUDE_ARTICLES_FROM_SORT) obsolete: > https://git.koha-community.org/Koha-community/Koha/commit/ > 0c9b39a66dff00d2d76effc42824c2051484f9c5 > > - Shouldn't the preference description omit the reference to serial search > results since the its use is theoretically general? > > - Is it necessary to replace [% INCLUDE subscriptions_table %] in > serials-search.tt? Going back to all that markup after switching to an > include seems like a step backwards. > > - Don't forget to run the QA tool to check for other issues after making > corrections. No they are not necessary. They have been removed. We are only using this patch on the serials search. So for now this description is good. Alexis, did you run the QA script on those patches? Created attachment 124104 [details] [review] Bug 19368 - Ignore words when sorting serial search This adds the 'SerialSortIgnoreWords' system preference. It is empty by default, since words to ignore are very much language-specific. The functionality is achieved by adding a hidden column to the table, and sorting by this column by default. The downfall of this solution is that if the user sorts by another column, they cannot go back to this sorting without refreshing the page. To test: 0) Apply patch 1) Run updatedatabase.pl 2) Add test subscriptions if you have none. Make sure to have subscriptions starting with "The An A" or similar 3) Go to system preferences and edit the SerialSortIgnoreWords pref Add the words "The An A" or the ones you used in your subscriptions 4) Go to Serials 5) Search serials (empty field will return all serials) 6) Result table should be sorted ignoring leading "The An A..." Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124105 [details] [review] Bug 19368 - Correction on QA feedback * I removed the IGNORE in the atomicupdate SQL. There is currently no other systempreference by that name in Master so that should work fine - Removed leftover test values from syspref.sql - Removed extra $ in serial-search.tt - Removed leftover warn in serial-search.pl Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124106 [details] [review] Bug 19368 - Syspref now SortIgnoreArticles, used in datatables.inc Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124107 [details] [review] Bug 19368 - Removed the old preference 'SerialSortIgnoreWords' that was left hanging in serials.pref Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124108 [details] [review] Bug 19368 - clearly state, in the pref's desc, that the comparison is done against unaccented title Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124109 [details] [review] Bug 19368 - Added missing closing bracket at the end of serials-search.pl Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 124110 [details] [review] Bug 19368 - (QA follow-up) Correction on QA feedback Created attachment 124111 [details] [review] Bug 19368 - Deleting useless variables in datatables.inc I tried to understand where it was used but I finally came to the conclusion that this is useless. (In reply to Jonathan Druart from comment #88) > Alexis, did you run the QA script on those patches? Yes I did. I missed one. I just fixed it. But the other one is just a git manipulation error and I don't know how to fix it. There are some spelling errors too. You need to amend the commits and fix their title: % git rebase -i HEAD~X (with X the number of commit) Replace "pick" with "r" (reword), save. The way this enhancement is implemented sounds weird to me. Why are you using perl when it can be done using Datatables (client-side)? Having the ability to define a list of word to ignore is great, but it can be done (at first glance) easily by extending the 'anti-the' plugin (and it will apply to *all* tables of Koha). |