https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30719
Katrin Fischer <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA QA Contact| |[email protected] --- Comment #95 from Katrin Fischer <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
