|
Description
simith.doliveira
2014-08-07 18:11:06 UTC
Created attachment 30592 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date This fix permits to sort late orders by basket creation or closing date Modified: installer/data/mysql/sysprefs.sql -AcqLateOrderUseCreationDate preference added koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref -AcqLateOrderUseCreationDate preference added koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt -Change "Order date" column acqui/lateorders.pl - C4/Acquisition.pm -Change sql Testing: I Apply the patch II Run updatedatabase.pl 0) Create 2 orders each one with different baskets; 1) Change the first basket date (table aqbasket, column creationdate and closedate) to 2014-07-13; 2) Change the second basket creationdate to 2014-07-12 and closedate to 2014-07-14; 3) Set AcqLateOrderUseCreationDate to Don't use the creation date of the basket to sort late order; 4) Validate sort by "close date" (second basket came first) and "Order close date" column; 5) Set AcqLateOrderUseCreationDate to Do use the creation date of the basket to sort late order; 6) Validate sort by "creation date" (first basket came first) and "Order creation date" column; sponsored by the CCSR ( http://www.ccsr.qc.ca ) I've applied the patch against master 3.17.00.013 I pass the patch to "Signed Off" status. Created attachment 30794 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date Comment on attachment 30794 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date Review of attachment 30794 [details] [review]: ----------------------------------------------------------------- Hi Simith, a couple of things: 0) The database update is missing from the patches. 1) Changes to core routines should be backed up by regression tests. ::: C4/Acquisition.pm @@ +2123,5 @@ > my @query_params = (); > my $select = " > SELECT aqbasket.basketno, > aqorders.ordernumber, > + DATE(aqbasket.${aqbasketSortField}) AS orderdate, 2) I think the variable shouldn't be in the SQL, but be replaced by a placeholder. @@ +2162,4 @@ > $select .= " > aqorders.quantity - COALESCE(aqorders.quantityreceived,0) AS quantity, > (aqorders.quantity - COALESCE(aqorders.quantityreceived,0)) * aqorders.rrp AS subtotal, > + DATEDIFF(CAST(now() AS date),${aqbasketSortField}) AS latesince Same here... @@ +2166,3 @@ > "; > if ( defined $delay ) { > + $from .= " AND (${aqbasketSortField} <= DATE_SUB(CAST(now() AS date),INTERVAL ? DAY)) " ; ... and here... @@ +2182,3 @@ > "; > if ( defined $delay ) { > + $from .= " AND (${aqbasketSortField} <= (CAST(now() AS date) -(INTERVAL ? DAY)) "; ... and here. ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt @@ +111,4 @@ > [% ELSE %] > <th></th> > [% END %] > + <th class="title-string">Order [% lateOrderSort %] date</th> 3) This is a translation problem. Please avoid "Order [IFELSE construct] date" in favor of repeating the whole line for each possible setting - This will allow translators to create better translations. ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref @@ +62,5 @@ > + choices: > + creation: Do > + closing: Don't > + - Use the creation date of the basket to sort late order. > + 4) Instead of a boolean, it would be nicer to rephrase a bit and use creation/closed date - Right now it is not clear at all what will be used instead of the creation date. Hi Katrin,
Concerning your comments:
0) Added in the new patch.
1) Ok for t/db_dependent/Acquisition.t and t/db_dependent/Acquisition.
2) I didn't know why, but using a placeholder in this code "DATEDIFF(CAST(now() AS date),${aqbasketSortField})" I got a different result, so I did not change it.
3) and 4) Changed in the new patch.
Created attachment 31064 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date - This patch fixs the numbers 0, 3 and 4 in comment 4 Add: installer/data/mysql/updatedatabase.pl Modified: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref - AcqLateOrderUseCreationDate preference modified. koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt - translation problem fixed. (In reply to simith.doliveira from comment #5) > 2) I didn't know why, but using a placeholder in this code > "DATEDIFF(CAST(now() AS date),${aqbasketSortField})" I got a different > result, so I did not change it. I was not sure either so asked Jared to take a look - it won't work because it's a column name. So it's probably ok. I am switching back to 'signed off' treating the second patch as a QA follow-up. Why don't you add the 2 infos in the table and let datatable does the sort job? The change in the subroutine needs unit tests. Switch to Failed QA. Created attachment 32298 [details]
updatedatabase.pl updated
Created attachment 32301 [details] Bug 12732 - Sort late orders by basket creation or closing date Created attachment 32303 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date - This patch fixs the numbers 0, 3 and 4 in comment 4 Add: installer/data/mysql/updatedatabase.pl Modified: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref - AcqLateOrderUseCreationDate preference modified. koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt - translation problem fixed. Sorry for the last 3 comments, I got some problems to fix merge conflict in installer/data/mysql/updatedatabase.pl - Regarding your question: Why don't you add the 2 infos in the table and let datatable does the sort job? The sponsor wanted just one column and the possibility to change the sort - The change in the subroutine needs unit tests. t/db_dependent/Acquisition.t alread has the tests for the subroutine that I changed (In reply to Jonathan Druart from comment #8) > Why don't you add the 2 infos in the table and let datatable does the sort > job? I agree with this suggestion. Can you please address this? I feel like this is case of YASP when we could probably allow this default sorting to be changed via some javascript in intranetuserjs by splitting the data into separate table columns. Kyle, This enhancement is sponsored by CCSR and that is the way they use. If the code is not suitable, so it must be followed up by someone else. Created attachment 58232 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date This fix permits to sort late orders by basket creation or closing date Modified: installer/data/mysql/sysprefs.sql -AcqLateOrderUseCreationDate preference added koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref -AcqLateOrderUseCreationDate preference added koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt -Change "Order date" column acqui/lateorders.pl - C4/Acquisition.pm -Change sql Testing: I Apply the patch II Run updatedatabase.pl 0) Create 2 orders each one with different baskets; 1) Change the first basket date (table aqbasket, column creationdate and closedate) to 2014-07-13; 2) Change the second basket creationdate to 2014-07-12 and closedate to 2014-07-14; 3) Set AcqLateOrderUseCreationDate to Don't use the creation date of the basket to sort late order; 4) Validate sort by "close date" (second basket came first) and "Order close date" column; 5) Set AcqLateOrderUseCreationDate to Do use the creation date of the basket to sort late order; 6) Validate sort by "creation date" (first basket came first) and "Order creation date" column; sponsored by the CCSR ( http://www.ccsr.qc.ca ) Created attachment 58233 [details] [review] Bug 12732 - Sort late orders by basket creation or closing date - This patch fixs the numbers 0, 3 and 4 in comment 4 Add: installer/data/mysql/updatedatabase.pl Modified: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref - AcqLateOrderUseCreationDate preference modified. koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt - translation problem fixed. The patches apply right now, but 3 things: 1) there are still no unit tests for the changes to the core routine 2) if we decide to have a system preference for this kind of behaviour, I'd suggest to rename it from AcqLateOrderUseCreationDate to AcqLaterOderDefaultSort which be more descriptive. 3) The SQL in sysprefs.sql is wrong: FAIL installer/data/mysql/sysprefs.sql OK git manipulation FAIL semicolon simicolon found instead of comma at line 6 OK sysprefs_order I agree with previous commenters that it doesn't seem quite right to switch the dates in the column like this. I'd like it if both were shown and the sorting handled by JavaScript. I think it would add valuable information and allow for more flexibility. We can now configure the default sort in administration > table configuration. I think all we need now is to add the entry date of the baskets as a new column. Created attachment 153617 [details] [review] Bug 12732: Add basket creation date to late orders table for sorting This patch modifies the late orders page so that the table of information contains a column for basket creation date. This will allow administrators to configure the table to sort by default on basket creation date if they wish. To test, apply the patch and restart services. - Go to Acquisitions -> Late orders. - The table of late orders should include a column labeled "Basket date" showing the basket creation date. - The dates should be formatted correctly and the column should sort correctly. - Go to Administration -> Table settings and navigate to the settings for the lateorders page. - Try changing various settings for the table to confirm that everything works correctly, including setting the default sort to use the basket date column. Created attachment 153618 [details]
SQL update to add late orders
Since the sample data doesn't contain late orders by default I made up this update
Created attachment 154633 [details] [review] Bug 12732: Add basket creation date to late orders table for sorting This patch modifies the late orders page so that the table of information contains a column for basket creation date. This will allow administrators to configure the table to sort by default on basket creation date if they wish. To test, apply the patch and restart services. - Go to Acquisitions -> Late orders. - The table of late orders should include a column labeled "Basket date" showing the basket creation date. - The dates should be formatted correctly and the column should sort correctly. - Go to Administration -> Table settings and navigate to the settings for the lateorders page. - Try changing various settings for the table to confirm that everything works correctly, including setting the default sort to use the basket date column. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Thanks for the patch on this long-outstanding bug! Just one thing: this patch uses the data-sort attribute in the td tag, but the same table has other rows that use data-order instead. Can this one be adjusted to use data-order for consistency? Created attachment 156280 [details] [review] Bug 12732: (follow-up) Use data-order instead of data-sort Created attachment 156291 [details] [review] Bug 12732: Add basket creation date to late orders table for sorting This patch modifies the late orders page so that the table of information contains a column for basket creation date. This will allow administrators to configure the table to sort by default on basket creation date if they wish. To test, apply the patch and restart services. - Go to Acquisitions -> Late orders. - The table of late orders should include a column labeled "Basket date" showing the basket creation date. - The dates should be formatted correctly and the column should sort correctly. - Go to Administration -> Table settings and navigate to the settings for the lateorders page. - Try changing various settings for the table to confirm that everything works correctly, including setting the default sort to use the basket date column. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Created attachment 156292 [details] [review] Bug 12732: (follow-up) Use data-order instead of data-sort Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x |