https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10190 --- Comment #955 from Chloé Zermatten <chloe.zermatten@openfifth.co.uk> --- (In reply to Jonathan Druart from comment #953)
Very quick, and late, QA review:
1. admin/circulation_triggers.tt 34 const letters = [% To.json(letters) | $raw %];
=> XSS?
2. admin/circulation_triggers.tt 37 const logged_in_library_id = "[% Branches.GetLoggedInBranchcode | html %]"; We have a global logged_in_user JS variable already
3. Koha/REST/V1/CirculationRules.pm 120 return $c->render_invalid_parameter_value( 121 { 122 path => '/query/rules', 123 values => { 124 uri => '/api/v1/kinds', 125 field => 'rule_name' 126 } 127 } 128 ) unless $valid_kinds->{$rule_key};
Is this correct? /kinds is not defined in the api specs. Should it be /circulation_rules/kinds?
4. Certainly a personal pov but the code in Koha/REST/V1/CirculationRules.pm starting at l.244 (`my $logged_in_user = $c->stash('koha.user');`) to return the 403 is confusing. Too many return statements maybe? I am also not sure it's covered properly by tests (but I may have missed them).
5. Why do we still have occurrences of "edit_notice_status_triggers" in t/Koha/Auth/Permissions.t?
t/Koha/Auth/Permissions.t:# Remove the deprecated edit_notice_status_triggers permission (bug 10190). t/Koha/Auth/Permissions.t:C4::Context->dbh->do(q|DELETE FROM permissions WHERE code = 'edit_notice_status_triggers'|);
6. i18n a. CirculationTriggersFormConfirmTriggerDelete.vue Untranslatable strings? + :formTitle=" + initialized + ? `Confirm deletion of Trigger ${triggerNumber} for ${handleContext(library_id, libraries, 'library_id')}` + : 'Confirm circulation trigger deletion' + " b. EditActions.vue + {{ $__("Add new trigger") + " " + triggerNumber }} + {{ $__("Edit trigger") }} {{ " " + triggerNumber }} => Should use `$__x()` to correctly handle this translation. Same pattern exist in other files.
7. EditActions.vue use an svg icon, any good reason or that? Doesn't it exist in font-awesome?
8. Duplicated CSS rules a. Just one example, but there are many like that: EditActions.vue CirculationTriggersFormConfirmTriggerDelete.vue CirculationTriggersFormConfirmReset.vue All 3 have the same CSS rules for `.chevron-buttons`, and only EditActions.vue have the .chevron-buttons element in the template. b. v-select related rules already exist in the shared css/vue.css (see "Bug 38201: Do not duplicated CSS rules")
9. In (at least) EditNotice.vue, there are !== conditions that can certainly be simplified: `v-if !== '' && !== null && !== undefined` => simply "v-if"? or do we care about 0? If we care about 0, then `!== null, !== undefined` and `!= null` are equivalent.
10. I don't understand why all the functions are in the stores file, is this a vue best practice?
Many thanks for this! Went through the list, and made changes accordingly. Here is where I landed: 1: Agreed, fixed 2: logged_in_user is not available to circulation triggers as it stands. I could make it available, but it also is a lot more data than what circulation triggers really needs. That said we do need to move from consulting the logged in library to looking for the logged in user’s home library instead - and that much is fixed (see commit message). 3: Agreed, fixed 4: I think we could extract code and collapse guard clauses 2 and 3 - but reckon that's a later follow-up? Re-tests - I’m not seeing any that check the permissions logics and 403s either (part of this is on me, I spotted the missing clause, added it, and forgot to update the tests accordingly, my bad!) -> added them. 5: The comment between those two lines reads “It may still be present in DBs that pre-date the atomic update.” I don’t know if you’ve seen it (won’t come up if grepping I don't think?). Does that answer it? I can ask Martin about it otherwise. 6-8: Agreed, fixed. 9: left untouched for now. “” is ‘No letters' where admins explicitly want nothing sent for this context, while null / undefined indicates no override, fall back to the less specific rule set. Where ‘No letters’ is set, we don’t display the transport type options - hence the explicit '' check. I think some cases can definitely be simplified in a follow up (which can be included as preparation for bug 39756. I do remember running into issues much earlier on when I did try to simplify - I don’t mind having another go, but I do think it’d be better in a follow-up. 10: best practice: I don’t think so - but I do think it’s a step in that direction (the direction of travel so far had been to remove duplicate code, prevent redundant data (re)loads, remove duplicated logic, etc). Further refactoring into more specialised components will be beneficial. 39756 (long overdue cron) can certainly take that refactoring further. Thoughts? I’ll happily stand corrected on anything I got wrong! -- You are receiving this mail because: You are watching all bug changes.