|
Description
Martin Renvoize (ashimema)
2022-02-10 13:23:02 UTC
Created attachment 158808 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 158809 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 158810 [details] [review] Bug 30070: Update edifactmsgs to use the API Adding Tomas and Kyle to CC here. * I'd like your opinions on the locations of the API endpoint and mappings Tomas. * Kyle added as a bit of an EDI guru in the hopes you might test it for me as most people shy away from EDI for testing. Our test server has 10,400 lines in the table and it is taking about 40 seconds to display the page. I don't know how we could set up a sandbox with lots of messages, so perhaps we could see how the change works on our test server. (Our higher specification production server has 11,100 lines but it still takes about 35 seconds to draw the page). I'm testing this is some production data and I'm getting 500 errors. I think it's because Koha::Edifact::File presumes every file will have a vendor and basket, but that's not necessarily true. I've patched those two methods but then I get the error: omething went wrong when loading the table. 500: Internal Server Error. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Properties not allowed: basket, deleted, vendor. Can you try testing with an edi message that has a null basket number? Created attachment 158887 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Thanks for testing Kyle, good call on the relation change. I'll continue here later tonight, be great to see this little enhancement in. Ray, I'll work through the issues Kyle highlighted and then look at it getting it into your test system. Ha, I just missed the final addition of the relations in the spec file. I'm out at the minute, but just adding vendor, deleted and basket keys to the edi_file spec should get this going. Created attachment 158903 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 158904 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 158905 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 158906 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 158907 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 158908 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 158909 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 158910 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 158911 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 158912 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 158913 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 158914 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 158915 [details] [review] Bug 30070: Add unit tests Created attachment 158918 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Created attachment 158920 [details] [review] Bug 30070: (follow-up) Fix javascript errors I fixed some more js issues but there are still a few outstanding issues: * The table is showing all edifact messages including deleted messages. I image some kind of search paramter needs to be added in Koha::REST::V1::Acquisitions::Edifact::Files::list * I get the following error on some but not all datatable pages: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. For more information about this error, please see http://datatables.net/tn/4 datatables_23.0600079.js:514:13 * The bookseller id is missing from the vendor links in the "Vendor" column Created attachment 158922 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 158923 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 158924 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 158925 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 158926 [details] [review] Bug 30070: Add unit tests Created attachment 158927 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Created attachment 158928 [details] [review] Bug 30070: (follow-up) Fix javascript errors Created attachment 158929 [details] [review] Bug 30070: (follow-up) Further javascript fixes This fixes an issue with the datatables when a message file is found to be missing the linked vendor or basket details. This removes the datatables error: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. Created attachment 158930 [details] [review] Bug 30070: (follow-up) Filter out deleted files on the API EDI uses a 'deleted' flag in the table to denote deleted files, we should filter those out of the API response. Created attachment 158931 [details] [review] Bug 30070: Improve view message modal display The display had some old specific styling added, we can just use modal-lg from the bootstrap library now to make the display more consistent with other modals. Created attachment 158932 [details] [review] Bug 30070: Remove 'Configure' option from datatable This passes 0 for table_settings effectively removing the 'Configure' option from the table. We haven't set up the background logic for table settings for this table and I don't believe there's a real use case for that here at the moment. Created attachment 158933 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 158934 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 158935 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 158936 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 158937 [details] [review] Bug 30070: Add unit tests Created attachment 158938 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Created attachment 158939 [details] [review] Bug 30070: (follow-up) Fix javascript errors Created attachment 158940 [details] [review] Bug 30070: (follow-up) Further javascript fixes This fixes an issue with the datatables when a message file is found to be missing the linked vendor or basket details. This removes the datatables error: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. Created attachment 158941 [details] [review] Bug 30070: (follow-up) Filter out deleted files on the API EDI uses a 'deleted' flag in the table to denote deleted files, we should filter those out of the API response. Created attachment 158942 [details] [review] Bug 30070: Improve view message modal display The display had some old specific styling added, we can just use modal-lg from the bootstrap library now to make the display more consistent with other modals. Created attachment 158943 [details] [review] Bug 30070: Remove 'Configure' option from datatable This passes 0 for table_settings effectively removing the 'Configure' option from the table. We haven't set up the background logic for table settings for this table and I don't believe there's a real use case for that here at the moment. Created attachment 158944 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158945 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158946 [details] [review] Bug 30070: Update edifactmsgs to use the API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158947 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158948 [details] [review] Bug 30070: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158949 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158950 [details] [review] Bug 30070: (follow-up) Fix javascript errors Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158951 [details] [review] Bug 30070: (follow-up) Further javascript fixes This fixes an issue with the datatables when a message file is found to be missing the linked vendor or basket details. This removes the datatables error: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158952 [details] [review] Bug 30070: (follow-up) Filter out deleted files on the API EDI uses a 'deleted' flag in the table to denote deleted files, we should filter those out of the API response. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158953 [details] [review] Bug 30070: Improve view message modal display The display had some old specific styling added, we can just use modal-lg from the bootstrap library now to make the display more consistent with other modals. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158954 [details] [review] Bug 30070: Remove 'Configure' option from datatable This passes 0 for table_settings effectively removing the 'Configure' option from the table. We haven't set up the background logic for table settings for this table and I don't believe there's a real use case for that here at the moment. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158955 [details] [review] Bug 30070: (QA follow-up) Make qa script happy Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> This is now up and running on your test server Ray. Let me know if you spot any issues or if you're happy I can add your signoff here and we can upgrade Kyles to a PQA stamp and get this pushed upstream :) Had my stopwatch ready to see how much of a reduction from 35-40 seconds I would see, but the EDIFACT messages page appeared almost instantaneously. I can't see any problems- delete action works and navigation to the message and back to other pages is just fine. Excellent, thanks for testing Ray :) I'll go ahead and add your signoff line for you. Created attachment 158963 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158964 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158965 [details] [review] Bug 30070: Update edifactmsgs to use the API Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158966 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158967 [details] [review] Bug 30070: Add unit tests Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158968 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158969 [details] [review] Bug 30070: (follow-up) Fix javascript errors Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158970 [details] [review] Bug 30070: (follow-up) Further javascript fixes This fixes an issue with the datatables when a message file is found to be missing the linked vendor or basket details. This removes the datatables error: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158971 [details] [review] Bug 30070: (follow-up) Filter out deleted files on the API EDI uses a 'deleted' flag in the table to denote deleted files, we should filter those out of the API response. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158972 [details] [review] Bug 30070: Improve view message modal display The display had some old specific styling added, we can just use modal-lg from the bootstrap library now to make the display more consistent with other modals. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158973 [details] [review] Bug 30070: Remove 'Configure' option from datatable This passes 0 for table_settings effectively removing the 'Configure' option from the table. We haven't set up the background logic for table settings for this table and I don't believe there's a real use case for that here at the moment. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158974 [details] [review] Bug 30070: (QA follow-up) Make qa script happy Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 158975 [details] [review] Bug 30070: (follow-up) Ensure strings are translatable Some strings moved from inline in templates to embedded in JS so we need to wrap them in _() now. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> My apologies, but one thing I should have noticed in testing was the unexpected order of results delivered. Prior to the patch I believe the sort was on column 2 (Transferred), with newest at the top, but now it appears to be on column 1 (Type). Certainly the ordering for us goes INVOICE > ORDERS > ORDRSP > QUOTE. Within those I don't know what further ordering is based on. Default sorting based on Transferred (newest at the top) would be the preferred option I think- it certainly is mine. Created attachment 159026 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 159027 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 159028 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 159029 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 159030 [details] [review] Bug 30070: Add unit tests Created attachment 159031 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Created attachment 159032 [details] [review] Bug 30070: (follow-up) Fix javascript errors Created attachment 159033 [details] [review] Bug 30070: (follow-up) Further javascript fixes This fixes an issue with the datatables when a message file is found to be missing the linked vendor or basket details. This removes the datatables error: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. Created attachment 159034 [details] [review] Bug 30070: (follow-up) Filter out deleted files on the API EDI uses a 'deleted' flag in the table to denote deleted files, we should filter those out of the API response. Created attachment 159035 [details] [review] Bug 30070: Improve view message modal display The display had some old specific styling added, we can just use modal-lg from the bootstrap library now to make the display more consistent with other modals. Created attachment 159036 [details] [review] Bug 30070: Remove 'Configure' option from datatable This passes 0 for table_settings effectively removing the 'Configure' option from the table. We haven't set up the background logic for table settings for this table and I don't believe there's a real use case for that here at the moment. Sorry, this no longer applies for me, but could it be we are missing patches? It starts right with a QA follow-up. Can you please check and fix? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 30070: (QA follow-up) Make qa script happy error: sha1 information is lacking or useless (Koha/REST/V1/Acquisitions/Vendor/Edifact/Files.pm). error: could not build fake ancestor Patch failed at 0001 Bug 30070: (QA follow-up) Make qa script happy hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-30070-QA-follow-up-Make-qa-script-happy-1cvqbapq.patch Created attachment 160336 [details] [review] Bug 30070: Add Koha Objects for EDI Message Files This patch adds the Koha Object based classes assocaited with the edifact_message table. This table actually contains the raw message files which may well each contain multiple edifact messages. Created attachment 160337 [details] [review] Bug 30070: Add fetch endpoint for EDI Files This patch adds a simple EDI Files listing REST endpoint Created attachment 160338 [details] [review] Bug 30070: Update edifactmsgs to use the API Created attachment 160339 [details] [review] Bug 30070: (follow-up) Return unless the related object exists Created attachment 160340 [details] [review] Bug 30070: Add unit tests Created attachment 160341 [details] [review] Bug 30070: Add koha_obejct_class(s) to EdifactMessage Created attachment 160342 [details] [review] Bug 30070: (follow-up) Fix javascript errors Created attachment 160343 [details] [review] Bug 30070: (follow-up) Further javascript fixes This fixes an issue with the datatables when a message file is found to be missing the linked vendor or basket details. This removes the datatables error: DataTables warning: table id=edi_msgs - Requested unknown parameter '4' for row 12, column 4. Created attachment 160344 [details] [review] Bug 30070: (follow-up) Filter out deleted files on the API EDI uses a 'deleted' flag in the table to denote deleted files, we should filter those out of the API response. Created attachment 160345 [details] [review] Bug 30070: Improve view message modal display The display had some old specific styling added, we can just use modal-lg from the bootstrap library now to make the display more consistent with other modals. Created attachment 160346 [details] [review] Bug 30070: Remove 'Configure' option from datatable This passes 0 for table_settings effectively removing the 'Configure' option from the table. We haven't set up the background logic for table settings for this table and I don't believe there's a real use case for that here at the moment. Created attachment 160347 [details] [review] Bug 30070: (QA follow-up) Make qa script happy Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 160348 [details] [review] Bug 30070: (follow-up) Ensure strings are translatable Some strings moved from inline in templates to embedded in JS so we need to wrap them in _() now. Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> *ping* Still waiting for a rebase. (In reply to Katrin Fischer from comment #102) > *ping* Still waiting for a rebase. The patch set is applying to current master cleanly for me! Are you getting conflicts? It applies, not sure what went wrong here, testing now. This appears to be pushed to master? I think I messed the bugzilla step or typoed "yes" again - I'll check in the morning. Can this be backported to 23.11.x ? We have it in our local 23.11 branch without causing any issues.. it's a performance enhancement, but I'd be happy to see it. We have had this running on 22.11.11 since November (see my Comment 76). Some further fixes (QA-related?) appear to have been added since mid-November 2023 and I am not sure if those have been applied to us on 21.11.11 but the backporting at 11/2023 worked fine then and other than the sorting issue I missed in testing (!), the performance improvement is major. We have 12,000 lines and the data apppears instantaneously rather than taking (I recall) 2 minutes or so. Further fixes were backported to your 21.11 Ray ;) For the rest of the PTFS customers, it's only ported back to 22.11.. In both cases, the backport was fairly trivial but for community I think I'd only recommend porting one version back here. Thanks Martin- nice to know we have had the additional patches applied to our system. But annoyingly I made a typo in my last comment. When I said "I am not sure if those have been applied to us on 21.11.11 [...] I should have said "I am not sure if those have been applied to us on 22.11.11 [...]". We have been on 22.11 since July 2023. Apologies. Pushed for 24.05! Well done everyone, thank you! (In reply to Martin Renvoize (ashimema) from comment #110) > but for community I think I'd only > recommend porting one version back here. Passage of time has put it back one version now, two versions in eight more days, is that far enough back? (In reply to Phil Ringnalda from comment #113) > (In reply to Martin Renvoize (ashimema) from comment #110) > > but for community I think I'd only > > recommend porting one version back here. > > Passage of time has put it back one version now, two versions in eight more > days, is that far enough back? This patch is from last cycle, so pushed to main was the correct status. What was missing was the RMaint closing this one off when coming past it. Closing it now. |