Some libraries would like to manage a "new" status for items. This status should be removed depending on some conditions.
Created attachment 21926 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules.
Created attachment 21932 [details] [review] Bug 11023: Toggle "new" status
Created attachment 21982 [details] [review] Bug 11023: Toggle "new" status
Created attachment 21983 [details] [review] Bug 11023: Toggle "new" status
Created attachment 22004 [details] [review] Bug 11023: Toggle "new" status
Created attachment 22386 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules.
There were been several iterations with Laurence. She found some bugs I fixed. This last patch is the final version.
Created attachment 22734 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table.
Created attachment 22735 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t
Created attachment 24068 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules.
Created attachment 24069 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table.
Created attachment 24070 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t
rebased patches.
Patch tested with a sandbox, by juliette et rémy <juliette.levast@iepg.fr>
Created attachment 24146 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et r
Created attachment 24147 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et r
Created attachment 24148 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et r
Is there any reason this couldn't be done using dateaccessioned rather than adding a new field to items? I imagine the test for items being 'new' is less than simple in some cases, and I can see a definite advantage in pre-setting items as new rather than determining that on the fly for every item. QA Issue: biblioitems_columns doesn't seem like it belongs in Items.pm, you could easily use DBIx::Class to get the columns names: http://search.cpan.org/~frew/DBIx-Class-0.08121/lib/DBIx/Class/ResultSource.pm#columns so no need for a special subroutine!
(In reply to Kyle M Hall from comment #18) > Is there any reason this couldn't be done using dateaccessioned rather than > adding a new field to items? I imagine the test for items being 'new' is > less than simple in some cases, and I can see a definite advantage in > pre-setting items as new rather than determining that on the fly for every > item. I am not sure to understand: the goal of the cronjob script is to remove the 'new' flag.
And the script is based on the items.dateaccessioned field for the duration value.
Created attachment 25767 [details] [review] Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue.
Created attachment 25833 [details] [review] Bug 11023: Use DBIx-Class to retrieve column names
(In reply to Kyle M Hall from comment #18) > QA Issue: biblioitems_columns doesn't seem like it belongs in Items.pm, you > could easily use DBIx::Class to get the columns names: > http://search.cpan.org/~frew/DBIx-Class-0.08121/lib/DBIx/Class/ResultSource. > pm#columns > so no need for a special subroutine! Done in the last patch. I kept the subroutines because I use the column names in 2 different places.
Firstly two merge conflicts: (got it working tho) CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Cronjob seems to work as intended and is a welcome addition. The purpose of the items.new-column is unclear. I didn't manage to get it populated, even if the comments here say that the cronjob's purpose is to empty the new-column. Apparently the new-column is a field, but then again it is a flag, and in the CREATE TABLE directive it is a VARCHAR(32). It is also the (status == new). It is extremely confusing. Apparently we can use it as we want, but then shouldn't it be labeled as such. My suggestion, replace the new-column definition with this: CREATE TABLE... `system_note` VARCHAR(32) DEFAULT NULL, -- You can put whatever free-text information regarding this item here. This field is meant for local modifications only and should not be used in features intended for master ... ;
Created attachment 26817 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et r
Created attachment 26818 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et r
Created attachment 26819 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et r
Created attachment 26820 [details] [review] Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue.
Created attachment 26821 [details] [review] Bug 11023: Use DBIx-Class to retrieve column names
Created attachment 26822 [details] [review] Bug 11023: Update DB field description
(In reply to Olli-Antti Kivilahti from comment #24) I disagree, this field is created to manage a "new" status, so I think "new" is a good name :) I provided a followup to change the description. Last patches have been rebased against master. Thank you for testing Olli!
We can agree to that :) Good work!
(In reply to Jonathan Druart from comment #30) > Created attachment 26822 [details] [review] [review] > Bug 11023: Update DB field description Typo: "indented" should be "intended"
Created attachment 26855 [details] [review] Bug 11023: Update DB field description
Starting with some initial testing and notes on this: 1) I am not sure the 2 specialiced subroutines for getting the columns from items and biblioitems are needed - couldn't this be done in ToggleNewStatus with DBIC? Right now it seems like the patch is using a mix of old and new (see comment 18 from Kyle). +sub columns { +sub ToggleNewStatus { +sub biblioitems_columns { 2) The 'new' column In comment 19 you said the goal of the cronjob was to remove the new flag. I am unsure if I see the use of the new field in the code and the feature. To me it seems just like a new 'notes' field, that is not really accessible without further configuration. a) In order to be able to set the field, you need to add a Koha-2- MARC-Mapping and change the bibliographic frameworks. Only then you will be able to catalog it. The field won't be searchable as there is no index on it. If we want to add an index later, this will be difficult, as we can't tell which mappings people will have used. How will libraries learn about the existance of the new column? b) For a general purpose field varchar(32) seems a bit limited. c) It seems the feature is independend on the existance of the new column. The central column for the feature seems to be the existing dateaccessioned column instead. With the implications of a) I am not sure we should include the 'new' from the beginning without having a clearer definition on how to use it. 4) Features in documentation I think the documentation lists features more like in an ad, than like in a help file. It's not clear to the reader, how to achieve the described functionality. That's what I have come up with: + <li>know easily what are the new items in the catalogue.</li> This still requires SQL, so doesn't seem to depend on the feature. + <li>display an icon in the search results for new items.</li> The only way to do this I can come up with is limited to libraries using biblio level itypes and by switching the itemtype with the script. + <li>configure issuing rules depending the 'new' status.</li> I think this would also require switching the itemtype? + <li>get a RSS/Atom feeds on these new items.</li> I think this would require using a special value in an item column that is searchable with Zebra. So you can build a search in Zebra that can then be used for the RSS feed? 4) biblioitems a) I am worried about some columns that should not be substituted: biblionumber, itemnumber, barcode, totalissues, onloan ... I think those are potentially dangerous and should not be listed. b) To me it seems like the only goal of including biblioitems is the possibility to change the bibliotitems itemtype. As it is now, it also allows changing values like title, pages, etc. which borders on a record batch edit. To me this seems a bit misplaced here. Also, ToggleNewStatus seems to only do ModItem - but you can define substitutions for biblioitems? (see also 6) 5) GUI a) It's not possible to edit a single rule. In order to add a rule or edit a rule, you always have to use 'Edit' and then all rules will display, the form for adding the new rule is at the very bottom. It is not very comfortable. b) There are no mandatory fields, duration is optional, as are conditions and substitions. So you can define an empty rule. c) In all other item related forms we use the names defined for the columns in the default framework (at least that's where I think those come from). But this feature only shows the names of the database columns. I think this is because of 2 a)? 6) Exploding? I defined a mean rule: Duration: 1 (only 1 item in my database has an accessiondate from yesterday) Conditions: no condition Substitutions: biblioitems.publishercode = XXXX biblioitems.url = something biblioitems.itemtype = XXXX items.barcode = xxxx items.materials = xxxx I didn't expect much to happen, but I had misinterpreted the duration. Results: DBD::mysql::st execute failed: Unknown column 'biblioitems.publishercode' in 'field list' at /home/katrin/kohaclone/C4/Items.pm line 2324. ERROR in _koha_modify_item UPDATE items SET biblioitems.publishercode=? WHERE itemnumber=?: Unknown column 'biblioitems.publishercode' in 'field list' at /home/katrin/kohaclone/C4/Items.pm line 2327. DBD::mysql::st execute failed: Unknown column 'biblioitems.url' in 'field list' at /home/katrin/kohaclone/C4/Items.pm line 2324. ERROR in _koha_modify_item UPDATE items SET biblioitems.url=? WHERE itemnumber=?: Unknown column 'biblioitems.url' in 'field list' at /home/katrin/kohaclone/C4/Items.pm line 2327. DBD::mysql::st execute failed: Unknown column 'biblioitems.itemtype' in 'field list' at /home/katrin/kohaclone/C4/Items.pm line 2324. ERROR in _koha_modify_item UPDATE items SET biblioitems.itemtype=? WHERE itemnumber=?: Unknown column 'biblioitems.itemtype' in 'field list' at /home/katrin/kohaclone/C4/Items.pm line 2327. DBD::mysql::st execute failed: Duplicate entry 'xxxx' for key 'itembarcodeidx' at /home/katrin/kohaclone/C4/Items.pm line 2324. ERROR in _koha_modify_item UPDATE items SET items.barcode=? WHERE itemnumber=?: Duplicate entry 'xxxx' for key 'itembarcodeidx' at /home/katrin/kohaclone/C4/Items.pm line 2327. Of course, no all my items have xxxx in materials. I thought initially only those items exactly 1 day old would be touched. But it will change all items older than 1 day. So this potentially will update a lot of items on each run. The SQL also shows that you can't actually update biblioitems columns, although the GUI offers the option. -------- I really like the idea of this, but to me it seems like there is some sort of mismatch between the feature description and how it seems to work. It feels like it needs some more thought. I think maybe this feature 'grew' since its initial specification? An example for a mismatch is also the description of the cronjob: +./toggle_new_status.pl -h + +Toggle recent acquisitions status. +Use this script to delete "new" status for items. There is no mention of the new column in the script. Another problem I see is that the feature as it is now is potentially quite dangerous and substitiutions on biblioitems don't work. Maybe biblioitems should be removed altogether from this. I'd love to hear some more opinions on this.
Just some more information on how the dev works before answering more in details to Katrin's notes The goal of the cronjob is to remove a "new" information from items after a given period. What could be a "new" information in the item : 1) a 'new' status added just to inform about the new document : we suppose an item subfield mapped with items.new field. This value can be used in search (if zebra index is configured), for custom rss feeds ; the subfield can be linked to an authorised value (authorised value=1, label=New). The cronjob will delete the value in the 'new' subfield. 2) a 'New' itype (itemtype) so that specific issuing rules can be defined for new items. With this configuration, final itemtype can be defined based on condition in another item subfield (ccode) or in a biblio subfield (mapped with biblioitems.itemtype) or in any items or biblioitems available field. The cronjob will delete the 'New' itype value and replace it by the suitable itype) 3) why not both of 1) and 2) or any item subfield the library wants to have some 'new' information. Only items are modified, the modification of biblio (biblio or biblioitems) is not part of the dev. Biblioitems fields are used only for the condition criteria. The items having dateaccessioned <= Now-duration will be modified by the job.
(In reply to Katrin Fischer from comment #35) Helle Katrin, > Starting with some initial testing and notes on this: > > 1) I am not sure the 2 specialiced subroutines for getting the > columns from items and biblioitems are needed - couldn't this > be done in ToggleNewStatus with DBIC? Right now it seems like > the patch is using a mix of old and new (see comment 18 from Kyle). There are 2 calls, 1 in C4::Items::ToggleNewStatus and 1 in the pl script. I don't think it is a good idea to call DBIC directly in pl files. > 2) The 'new' column > In comment 19 you said the goal of the cronjob was to remove the > new flag. > I am unsure if I see the use of the new field in the code and the > feature. To me it seems just like a new 'notes' field, that is not > really accessible without further configuration. As I tried to explain in the commit message, this feature allows to manage new items as you want. There is no restriction or a "way todo". I listed test case examples, it is how we use this field at BibLibre. This patch is an "introduction" to the idea of manage a "new" status. > a) In order to be able to set the field, you need to add a Koha-2- > MARC-Mapping and change the bibliographic frameworks. Only then > you will be able to catalog it. The field won't be searchable > as there is no index on it. If we want to add an index later, this > will be difficult, as we can't tell which mappings people will > have used. > How will libraries learn about the existance of the new > column? Reading the changelog? > b) For a general purpose field varchar(32) seems a bit limited. In a lot of cases, we used it as a boolean, so it is enough. What do you suggest, VARCHAR(64), TEXT,...? > c) It seems the feature is independend on the existance of the new > column. The central column for the feature seems to be the > existing dateaccessioned column instead. > With the implications of a) I am not sure we should include the > 'new' from the beginning without having a clearer definition > on how to use it. This field is important for the workflow of our users, it is part of the feature. > 4) Features in documentation > I think the documentation lists features more like in an ad, than > like in a help file. Sorry, it was not the expected goal. > It's not clear to the reader, how to achieve > the described functionality. That's what I have come up with: > > + <li>know easily what are the new items in the catalogue.</li> > > This still requires SQL, so doesn't seem to depend on the feature. It depends on the "new" field, so it depends on this feature, no? > + <li>display an icon in the search results for new items.</li> > > The only way to do this I can come up with is limited to libraries > using biblio level itypes and by switching the itemtype with the > script. Yes, it is one of the purpose of it. > + <li>configure issuing rules depending the 'new' status.</li> > > I think this would also require switching the itemtype? Yes. > + <li>get a RSS/Atom feeds on these new items.</li> > > I think this would require using a special value in an item > column that is searchable with Zebra. So you can build a search in > Zebra that can then be used for the RSS feed? I don't know how they do that, maybe using a sql query. > 4) biblioitems > > a) I am worried about some columns that should not be substituted: > biblionumber, itemnumber, barcode, totalissues, onloan ... > I think those are potentially dangerous and should not be listed. It is a cronjob script, all of them should be used with full knowledge of the facts. I think it is easier/better to list all fields rather than to limit the possibilities. > b) To me it seems like the only goal of including biblioitems is > the possibility to change the bibliotitems itemtype. As it is now, > it also allows changing values like title, pages, etc. which > borders on a record batch edit. To me this seems a bit misplaced > here. > Also, ToggleNewStatus seems to only do ModItem - but you can > define substitutions for biblioitems? (see also 6) It seems only conditions on biblioitems have been tested, we don't use biblioitems for substitutions. I will provide a patch to remove the biblioitems fields in the substitution list. > 5) GUI > > a) It's not possible to edit a single rule. In order to add a rule > or edit a rule, you always have to use 'Edit' and then all rules > will display, the form for adding the new rule is at the very > bottom. It is not very comfortable. What do you suggest? I tried to do something ergonomic with a quite complicated form. With the actual way, if you have to add several rules, it is useless to save after adding each one, but only once at the end. I chose to edit all rules on the same screen, because they are stored in a syspref and I would prefer not to parse the pref to update the rules. > b) There are no mandatory fields, duration is optional, as are > conditions and substitions. So you can define an empty rule. Yes, it could be possible if you want to mark all items as new. I really want to avoid a long list of follow-ups. I don't want to add all requests in this report. If some reports are opened and linked to this one, and they are relevant, I will provide a patch. Originally, this feature was something like http://git.biblibre.com/?p=koha;a=blob;f=misc/cronjobs/toggle_new_status.pl;h=5acad013fac5bbbe22d17dab4369f99e28dd93e3;hb=8aa5f86906774da52bd317acef4131d9142c2431 We decided to submit something more flexible and more complete for the community, in order to open the possibilities.
Created attachment 27810 [details] [review] Bug 11023: Don't use the biblioitems fields for the subtitution It's dangerous to allow a change on the biblioitems fields with this feature.
Not sure why this is failed qa, I think it can be back to signoff.
Created attachment 29216 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 29217 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 29218 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 29219 [details] [review] Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue.
Created attachment 29220 [details] [review] Bug 11023: Use DBIx-Class to retrieve column names
Created attachment 29221 [details] [review] Bug 11023: Don't use the biblioitems fields for the subtitution It's dangerous to allow a change on the biblioitems fields with this feature.
Created attachment 29222 [details] [review] Bug 11023: DB changes
I extracted the DB changes in a new patch. I also added the DBIx changes to it. It fixes a bug: the items.new field was not list in the field list.
(In reply to Jonathan Druart from comment #46) > Created attachment 29222 [details] [review] [review] > Bug 11023: DB changes Note: I don't know why this change has been generated: - on_delete => "CASCADE", + on_delete => "RESTRICT",
Just to support its integration. We use it a lot in production and for the moment we have to apply it manually, but patchs approved by BibLibre ;)
Issues: 1) Can enter an invalid days value ( for example 'test' rather than '5' ) in the editor 2) It would be better if the rules were listed in a table. The current display is very hard to read. 3) I think "Duration" should be "Age". Duration implies something taking place between now and the future, which isn't the case. I believe it's easier to think in terms of "When this item is 10 days old, do this". 3) I'm not sure if "Toggle new status" is a good name for this feature. Maybe something more along the lines of "Automatic item modifications by age". That's quite a mouthful, but I think it represents the feature better. The feature seems quite powerful, and I think the current name belies that fact.
Created attachment 31820 [details] [review] Bug 11023: Rename the duration parameter with 'age'
Created attachment 31821 [details] [review] Bug 11023: The age parameter should be a number The template should check if the age parameter is correctly filled (should be a number).
Created attachment 31822 [details] [review] Bug 11023: Change the name of the feature Originaly this feature only permits to update the "new" field. Now all item fields can be updated. The name of the feature is now "Automatic item modifications by age".
(In reply to Kyle M Hall from comment #50) Kyle, Thanks to QA this feature! > Issues: > 1) Can enter an invalid days value ( for example 'test' rather than '5' ) in > the editor Fixed, it's now a number. > 2) It would be better if the rules were listed in a table. The current > display is very hard to read. I am not sure, you can have several conditions and substitutions for 1 rule. I started with a table but I didn't manage to do something clear. > 3) I think "Duration" should be "Age". Duration implies something taking > place between now and the future, which isn't the case. I believe it's > easier to think in terms of "When this item is 10 days old, do this". Renamed. > 3) I'm not sure if "Toggle new status" is a good name for this feature. > Maybe something more along the lines of "Automatic item modifications by > age". That's quite a mouthful, but I think it represents the feature better. Renamed.
Created attachment 33226 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 33227 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 33228 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 33229 [details] [review] Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue.
Created attachment 33230 [details] [review] Bug 11023: Use DBIx-Class to retrieve column names
Created attachment 33231 [details] [review] Bug 11023: Don't use the biblioitems fields for the subtitution It's dangerous to allow a change on the biblioitems fields with this feature.
Created attachment 33232 [details] [review] Bug 11023: DB changes
Created attachment 33233 [details] [review] Bug 11023: Rename the duration parameter with 'age'
Created attachment 33234 [details] [review] Bug 11023: The age parameter should be a number The template should check if the age parameter is correctly filled (should be a number).
Created attachment 33235 [details] [review] Bug 11023: Change the name of the feature Originaly this feature only permits to update the "new" field. Now all item fields can be updated. The name of the feature is now "Automatic item modifications by age".
Fixed the first conflict on the tools home page, but then got stuck with a harder conflict: Apply? [(y)es, (n)o, (i)nteractive] i Applying: Bug 11023: Add the ability to specify fields from biblioitems table. Applying: Bug 11023: Add UT for C4::Items::ToggleNewStatus Applying: Bug 11023: FIX - condition on biblioitems table does not work Applying: Bug 11023: Use DBIx-Class to retrieve column names fatal: sha1 information is lacking or useless (C4/Items.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11023: Use DBIx-Class to retrieve column names The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/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-11023-Use-DBIx-Class-to-retrieve-column-names-J9vKlx.patch
Created attachment 34782 [details] [review] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 34783 [details] [review] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 34784 [details] [review] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et rémy <juliette.levast@iepg.fr>
Created attachment 34785 [details] [review] Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue.
Created attachment 34786 [details] [review] Bug 11023: Use DBIx-Class to retrieve column names
Created attachment 34787 [details] [review] Bug 11023: Don't use the biblioitems fields for the subtitution It's dangerous to allow a change on the biblioitems fields with this feature.
Created attachment 34788 [details] [review] Bug 11023: DB changes
Created attachment 34789 [details] [review] Bug 11023: Rename the duration parameter with 'age'
Created attachment 34790 [details] [review] Bug 11023: The age parameter should be a number The template should check if the age parameter is correctly filled (should be a number).
Created attachment 34791 [details] [review] Bug 11023: Change the name of the feature Originaly this feature only permits to update the "new" field. Now all item fields can be updated. The name of the feature is now "Automatic item modifications by age".
Patches rebased.
Created attachment 34870 [details] [review] [PASSED QA] Bug 11023: Toggle "new" status This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjob/toggle_new_status.pl). Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Toggle new status - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et remy <juliette.levast@iepg.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34871 [details] [review] [PASSED QA] Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et remy <juliette.levast@iepg.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34872 [details] [review] [PASSED QA] Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et remy <juliette.levast@iepg.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34873 [details] [review] [PASSED QA] Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34874 [details] [review] [PASSED QA] Bug 11023: Use DBIx-Class to retrieve column names Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34875 [details] [review] [PASSED QA] Bug 11023: Don't use the biblioitems fields for the subtitution It's dangerous to allow a change on the biblioitems fields with this feature. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34876 [details] [review] [PASSED QA] Bug 11023: DB changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34877 [details] [review] [PASSED QA] Bug 11023: Rename the duration parameter with 'age' Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34878 [details] [review] [PASSED QA] Bug 11023: The age parameter should be a number The template should check if the age parameter is correctly filled (should be a number). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34879 [details] [review] [PASSED QA] Bug 11023: Change the name of the feature Originaly this feature only permits to update the "new" field. Now all item fields can be updated. The name of the feature is now "Automatic item modifications by age". Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34880 [details] [review] Bug 11023 [QA Followup] * Update DB version * Fix capitalization error * Rename misc/cronjobs/toggle_new_status.pl to misc/cronjobs/automatic_item_modification_by_age.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34881 [details] [review] Bug 11023 [QA Followup] - Complete the renaming of "toggle new status" to "automatic item modification by age" Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34882 [details] [review] Bug 11023 [QA Followup] - Complete the renaming of "toggle new status" to "automatic item modification by age" Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34883 [details] [review] Bug 11023 [QA Followup] - Complete the renaming of "toggle new status" to "automatic item modification by age" Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
No intention to block this, but still a bit worried about the possibility of people doing changes that might cause problems. Would people be ok with adding a separate permission for this tool? Currently it's tied to the batch delete permission, but not working with the restricted batch delete: flagsrequired => { tools => 'items_batchmod' }, Offering to write the patch :)
(In reply to Katrin Fischer from comment #91) > No intention to block this, but still a bit worried about the possibility of > people doing changes that might cause problems. > > Would people be ok with adding a separate permission for this tool? > Currently it's tied to the batch delete permission, but not working with the > restricted batch delete: > > flagsrequired => { tools => 'items_batchmod' }, > > Offering to write the patch :) The items_batchmod permission make sense to me. I would prefer to add the support for items_batchmod_restricted if it makes sense for you too.
Guess I am still a little worried about people breaking things with this new tool - so that's why I'd have preferred a separate permission. We have a separate permission for every other tool, so it would fit in well. But the combination with the restricted mode makes things more complicated here. Not sure what's best. If we leave it with item batchmod it would make sense to add the restricted behaviour, but also sounds more complicated to do :) Maybe we should open a separate bug?
(In reply to Katrin Fischer from comment #93) > Maybe we should open a separate bug? It's certainly the best. While people will start to use it, we will get feedback to improve the existing behavior.
s/While/When
(In reply to Kyle M Hall from comment #90) > Created attachment 34883 [details] [review] [review] > Bug 11023 [QA Followup] - Complete the renaming of "toggle new status" to > "automatic item modification by age" > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I am not sure it's a good idea to create files and rename them in the same patch set. Maybe should we squash patches?
Created attachment 35375 [details] [review] Bug 11023: Automatic item modification by age (Was Toggle "new" status") This patch adds: - a new DB field items.new. - a new page to configure this new status (tools/toggle_new_status.pl). - a new cronjob script (misc/cronjobs/automatic_item_modification_by_age.pl was misc/cronjob/toggle_new_status.pl) Why this status is useful for some libraries ? The use cases are: - to know easily what are the new items (with a simple sql query). - to display an icon in the search results. - issuing rules can be adapt for new items. Automatically (using the cronjob script), the status change (depending the configuration) and the item can be issued, for example. - a RSS/Atom feeds can be created on these new items. Test plan: - log in with a librarian having the tools > items_batchmod permission. - navigate to Home > Tools > Automatic item modifications by age (was: Toggle new status) - click on the edit button - there are 3 "blocks": * duration: the duration during an item is considered as new. * conditions: the status will change only if the conditions are meet. * substitutions: if there is no substitution, no action will be done. You can add some change to apply to the matching items. E.g. ccode=3 new='' If the value is an empty string (in other words, the input does not contain anything), the field will be deleted. You can create as many rules as you want. - test the interface : add/remove rule, conditions, substitutions, submit the form, edit, etc. (There is a looot of JS everywhere, so certainly a looot of bugs...). - when you have your rules defined, you can now launch the cronjob script without any parameter. A report will be displayed with the matching itemnumber and the substitutions to apply. Verify results are consistent. - launch the script with the -c argument and verify values have been modified depending the substitution rules. Signed-off-by: juliette et remy <juliette.levast@iepg.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: Add the ability to specify fields from biblioitems table. Test plan: Same as before but try with fields from the biblioitems table. Signed-off-by: juliette et remy <juliette.levast@iepg.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: Add UT for C4::Items::ToggleNewStatus Test plan: prove t/db_dependent/Items/ToggleNewStatus.t Signed-off-by: juliette et remy <juliette.levast@iepg.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: FIX - condition on biblioitems table does not work If a rule contains a condition on the biblioitems table, the match won't work. This patch fixes this issue. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: Use DBIx-Class to retrieve column names Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: Don't use the biblioitems fields for the subtitution It's dangerous to allow a change on the biblioitems fields with this feature. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: Rename the duration parameter with 'age' Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: The age parameter should be a number The template should check if the age parameter is correctly filled (should be a number). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023: Change the name of the feature Originaly this feature only permits to update the "new" field. Now all item fields can be updated. The name of the feature is now "Automatic item modifications by age". Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023 [QA Followup] * Update DB version * Fix capitalization error * Rename misc/cronjobs/toggle_new_status.pl to misc/cronjobs/automatic_item_modification_by_age.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 11023 [QA Followup] - Complete the renaming of "toggle new status" to "automatic item modification by age" Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35376 [details] [review] Bug 11023: DB changes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Jonathan Druart from comment #96) > (In reply to Kyle M Hall from comment #90) > > Created attachment 34883 [details] [review] [review] [review] > > Bug 11023 [QA Followup] - Complete the renaming of "toggle new status" to > > "automatic item modification by age" > > > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> > > I am not sure it's a good idea to create files and rename them in the same > patch set. > Maybe should we squash patches? I squashed all patches, except the DB changes.
Can u please rebase it? I'm willing to test this ASAP.
Tomàs, Jajm should take it today. Thanks.
Created attachment 41147 [details] [review] Bug 11023: Automatic item modification by age (Was Toggle "new" status") Rebased on master
Created attachment 41148 [details] [review] Bug 11023: DB changes Rebased on master and Koha/Schema/* files removed from the patch to avoid conflicts
Hello, Can you tell me if this patch will be compatible with 3.18 ? We are very interested ^^
(In reply to Pierre Angot from comment #104) > Hello, > > Can you tell me if this patch will be compatible with 3.18 ? > > We are very interested ^^ At least one of the previous version was :) It won't be backported, it's an enh.
Pushed to Master - Should be in the May 2016 Release. Thanks!
I just had a thought about this. I think the db column 'new' is going to never be accessible via Koha::Item. I think a followup will be needed to change the column from 'new' to maybe 'new_status'? Any other column name suggestions would be appreciated!
(In reply to Kyle M Hall from comment #107) > I just had a thought about this. I think the db column 'new' is going to > never be accessible via Koha::Item. I think a followup will be needed to > change the column from 'new' to maybe 'new_status'? Any other column name > suggestions would be appreciated! See bug 16004.