[Koha-bugs] [Bug 30719] ILL should provide the ability to create batch requests
bugzilla-daemon at bugs.koha-community.org
bugzilla-daemon at bugs.koha-community.org
Fri Jun 23 14:55:10 CEST 2023
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30719
Katrin Fischer <katrin.fischer at bsz-bw.de> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|Signed Off |Failed QA
QA Contact| |katrin.fischer at bsz-bw.de
--- Comment #95 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
The dependency to bug 33716 is a bit of an issue here, we need to see how to
resolve that. As I signed it off I can't QA, but it will send this one to
BLOCKED even if passing.
I skipped the last patch (DBIC) and ran dbic manually for testing as there was
a conflict.
1) QA script
FAIL koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal.inc
FAIL valid_template
options_for_libraries: not found
FAIL installer/data/mysql/kohastructure.sql
FAIL boolean_vs_tinyint
WARNING - The new column (is_system) for table
illbatch_statuses is using INT(1) as type, must be TINYINT(1) if it has a
boolean purpose, see the SQL12 coding guideline
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
FAIL filters
wrong_html_filter at line 669 (
<a href="/cgi-bin/koha/ill/ill-requests.pl?batch_id=[% request.batch.id |
html %]">)
2a) Database update is not idempotent
Easy fix with our existing check methods:
DEV atomic update
/kohadevbox/koha/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl
[11:39:04]: Bug 30719 - Add ILL batches
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate column
name 'batch_id' at /kohadevbox/koha/C4/Installer.pm line 741
2b) Database update and kohastructure are not using COMMENT
We standardized on using COMMENT as they display in schema.koha-community.org
etc. Could you please move the comments to COMMENT syntax?
3) Unit tests are failing (after running db Update and dbic)
Test Summary Report
-------------------
t/db_dependent/api/v1/ill_requests.t (Wstat: 512 Tests: 2 Failed: 2)
Failed tests: 1-2
Non-zero exit status: 2
Files=5, Tests=33, 21 wallclock secs ( 0.03 usr 0.02 sys + 18.99 cusr 1.70
csys = 20.74 CPU)
Result: FAIL
4) Perltidy
It might be nice (not blocker) to run the new perltidy over the code when
working on it, thinking especially of the new files. There are some small
inconsistencies like spaces and such:
+use JSON qw( to_json );
+use base qw(Koha::Object);
5) FIXMEs
Koha/REST/V1/Illbatches.pm
+ #FIXME: This should be $c->objects-search
+ my @batches = Koha::Illbatches->search()->as_list;
+
+ #FIXME: Below should be coming from $c->objects accessors
+ # Get all patrons associated with all our batches
+# FIXME: This should be moved to Koha::Illbackend
+sub can_batch {
+ // TODO: need to also reset progress bar and already processed
identifiers
+ // FIXME: This should be a kohaTable not KohaTable
Are these something we should change now or at least have a follow-up bug for
filed?
6) API
If I understand correctly the API is not using the common/standardized
terminology:
+ id:
+ type: string
+ description: Internal ILL batch identifier
Should be batch_id or maybe ill_batch_id? (compared with other API routes:
hold_id, patron_id, etc.)
+ borrowernumber:
+ type: string
+ description: Borrower number of the patron of the ILL batch
Should be: patron_id
+ branchcode:
+ type: string
+ description: Branch code of the branch of the ILL batch
Should be library_id
+ branch:
+ type:
+ - object
+ - "null"
+ description: The branch associated with the batch
Should possibly be library
+ statuscode:
+ type: string
+ description: Code of the status of the ILL batch
Should probably be status_code
Similar for illbatchstatus.yaml:
id:
ill_batch_status_id ?
7a) Illbatch_statuses are not translatable
They should probably be moved to en, like we just did with the debit types and
credit types too. For reporting it's nice when the description can be
translated.
+++ b/installer/data/mysql/mandatory/illbatch_statuses.sql
@@ -0,0 +1,5 @@
+INSERT INTO illbatch_statuses ( name, code, is_system ) VALUES
+('New', 'NEW', 1),
+('In progress', 'IN_PROGRESS', 1),
+('Completed', 'COMPLETED', 1),
+('Unknown', 'UNKNOWN', 1);
7b) Strings can be transalted inside the .js files
We don't really need:
ill-batch-modal-strings.inc
ill-batch-table-strings.inc
This file is probably no longer needed as we can now translate strings in .js
files. Not blocker, could still be moved later. It might simplify the code in
koha-tmpl/intranet-tmpl/prog/js/ill-batch-modal.js quite a bit.
Also:
+ var ill_batch_item_remove = _("Are you sure you want to remove this item
from the batch");
Missing question mark at the end?
7c) Avoid "building" sentences from multiple parts
+ <a href="#" aria-current="page">
+ [% IF status.id %]
+ Modify
+ [% ELSE %]
+ New
+ [% END %] batch status
+ </a>
This will end up to be something like: %sModify%New%batch status. This is hard
to read, but also harder to translate. Imaging having a language where you need
to change the sequence, to be "Batch status edit". Better:
+ <a href="#" aria-current="page">
+ [% IF status.id %]
+ <span>Modify batch status</span>
+ [% ELSE %]
+ <span>New batch status</span>
+ [% END %]
+ </a>
The span would be to make both separate strings. The parsing looks for HTML
tags. repeating the <a> inside the IF-ELSE would also work.
Also here:
+ <h1>
+ View ILL requests
+ [% IF batch %]
+ for batch "[% batch.name | html %]"
+ [% END %]
+ </h1>
7d) Not sure if this is translatable
I have doubts about this one here:
<strong>[% status.is_system ? "Yes" : "No" | html %]</strong>
I think we need to verify or just move to an IF-ELSE construct.
8) Capitalization
+ <li><a
href="/cgi-bin/koha/admin/ill_batch_statuses.pl">Interlibrary Loan batch
statuses</a></li>
Interlibrary loan...
+ <th scope="col">Request Status</th>
Request status
<dt><a href="/cgi-bin/koha/admin/ill_batch_statuses.pl">Interlibrary Loan batch
statuses</a></dt>
+ Interlibrary Loan batch statuses › Administration › Koha
+ <a href="/cgi-bin/koha/admin/ill_batch_statuses.pl">Interlibrary Loan
batch statuses</a>
Maybe to a search replace for Interlibrary Loan? There are many more...
9) Terminology
+ <th scope="col">Branch</th>
Should be Library.
10) Page titles and breadcrumbs
Need updating to follow Owen's latest work using Template WRAPPER etc.
koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt
11) Using page-section and btn-primary
This predates the staff interface redesign work and needs some touch ups for
yellow main buttons and white backgrounds.
12) Null?
This looks like it might display "null" when either one is empty (unconfirmed):
+ var displayText = [data.firstname, data.surname].join(' ') + ' ( ' +
data.cardnumber + ' )';
--
You are receiving this mail because:
You are watching all bug changes.
More information about the Koha-bugs
mailing list