Bug 10190: (QA follow-up): fixes phase five

1. apply html filter to letters so notice names are escaped (must
    be done in attribute so the JSON format of the letters can
    be parsed).
2. fix a correctness issue where the frontend was pinning the
   logged in library instead of the logged in user's home library,
   which would have led to 403s (correct) when saving rules. Since
   the library dropdown is deliberately disabled unless a user has
   the manage_circ_rules_from_any_libraries permission, the user
   would then have needed to use 'Set library' to match their home
   library to fix this. Prevent the issue by setting the library
   to the user's home library.
   Also rename the variable to user_library_id.
3. list_rules correctly references /api/v1/circulation_rules/kinds
4. make untranslatable strings translatable
5. swap svg for font-awesome icon.
6. avoid css duplication:
   - remove dead / unused scoped css (leftover from an early component
     extraction)
   - import vue.css and only leave the minimal override required.
7. improve set_rules test coverage: add tests for manage_circ_rules
   / manage_circ_triggers separation

Test plan:

1) Administration > Circulation triggers: notice the 'Letter' dropdowns
   list circulation notices as "name (code)" and offer 'No letter'.
   (no regression)
2) Give a staff user 'catalogue' + 'manage_circ_triggers' only, with
   home library A. Log in as them, 'Set library' to B, and open
   Administration > Circulation triggers. Notice the view is pinned to
   A, and that adding and editing a trigger saves without a 403.
   (behaviour is now correct)
3) GET /api/v1/circulation_rules?rules=nosuchrule - notice the error
   points at /api/v1/circulation_rules/kinds.
   (behaviour is now correct)
4) Run 'gulp po:update --lang fr-FR'. Notice 'Add new trigger
   {triggerNumber}', 'No letter' and 'any' now appear in
   misc/translator/po/fr-FR-messages-js.po
   (behaviour is now correct)
5) Edit a trigger. Notice the delay's reset button is an 'x' inside the
   input, left of the chevrons, and clears the delay; the letter's
   reset button does the same for the notice.
   (no regression)
6) Notice no visual regression across the list, add, edit, reset and
   delete screens - select widths, chevrons and modals unchanged.

Also run t/db_dependent/api/v1/circulation_rules.t

Assisted-by: Claude Opus 5 (Anthropic)
Sponsored-by: Glasgow Colleges Library Group <https://library.cityofglasgowcollege.ac.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Chloé Zermatten 2026-09-11 09:35:24 +00:00 committed by Pedro Amorim
parent 167622c08d
commit c96618f064
No known key found for this signature in database
GPG Key ID: 06B75BA7F3894D55
12 changed files with 105 additions and 260 deletions

View File

@ -121,7 +121,7 @@ sub list_rules {
{
path => '/query/rules',
values => {
uri => '/api/v1/kinds',
uri => '/api/v1/circulation_rules/kinds',
field => 'rule_name'
}
}

View File

@ -30,11 +30,12 @@
<!-- FIXME: this shouldn't be needed -->
[% INCLUDE 'datatables.inc' %]
[% INCLUDE 'js-date-format.inc' %]
<div id="letters-data" style="display:none" aria-hidden="true" data-letters="[% To.json(letters) | html %]"></div>
<script nonce="[% Koha.CSPNonce | $raw %]">
const letters = [% To.json(letters) | $raw %];
const letters = JSON.parse(document.getElementById("letters-data").dataset.letters);
const default_view = "[%- IF Koha.Preference('DefaultToLoggedInLibraryOverdueTriggers') -%][% Branches.GetLoggedInBranchcode | html %][%- ELSE -%]*[%- END -%]";
const from_branch = "[% Koha.Preference('OverdueNoticeFrom') | html %]";
const logged_in_library_id = "[% Branches.GetLoggedInBranchcode | html %]";
const user_library_id = "[% logged_in_user.branchcode | html %]";
</script>
[% Asset.js("js/vue/dist/admin/circulation_triggers.js") | $raw %]
[% END %]

View File

@ -34,7 +34,7 @@
:editMode="editMode"
:canManageAnyLibrary="canManageAnyLibrary"
:libraries="libraries"
:logged_in_library_id="logged_in_library_id"
:user_library_id="user_library_id"
:patronCategories="patronCategories"
:itemTypes="itemTypes"
/>
@ -147,7 +147,7 @@ export default {
lastEditedTriggerNumber,
storeInitialized,
canManageAnyLibrary,
logged_in_library_id,
user_library_id,
} = storeToRefs(circRulesStore);
return {
@ -166,7 +166,7 @@ export default {
lastEditedTriggerNumber,
storeInitialized,
canManageAnyLibrary,
logged_in_library_id,
user_library_id,
compareByProperty,
scrollToElementById,
};
@ -332,8 +332,9 @@ export default {
this.triggerNumber
)
) {
this.alertMessage =
"Your changes could not be saved as this circulation trigger was updated elsewhere. Please see the updated trigger below.";
this.alertMessage = this.$__(
"Your changes could not be saved as this circulation trigger was updated elsewhere. Please see the updated trigger below."
);
this.$router.push({
path: "/cgi-bin/koha/admin/circulation_triggers/edit",
query: {
@ -380,8 +381,8 @@ export default {
this.ruleSetInitialized = true;
},
setContext(query) {
if (!this.canManageAnyLibrary && this.logged_in_library_id) {
this.context.library_id = this.logged_in_library_id;
if (!this.canManageAnyLibrary && this.user_library_id) {
this.context.library_id = this.user_library_id;
} else {
this.context.library_id = query.library_id ?? "*";
}

View File

@ -1,8 +1,8 @@
<template>
<CirculationTriggersForm
:submitAction="resetCircRules"
formTitle="Confirm circulation rule set reset"
buttonText="Confirm reset"
:formTitle="$__('Confirm circulation rule set reset')"
:buttonText="$__('Confirm reset')"
:disabled="resetWouldLeaveRuleSetWithoutDelay"
>
<TriggersTable
@ -198,98 +198,6 @@ form ol li {
float: none;
}
.numeric-input-wrapper {
position: relative;
display: inline-block;
width: 30%;
}
.input-with-clear {
position: relative;
display: flex;
align-items: center;
width: 100%;
}
.numeric-input {
padding-right: 40px; /* Adjust to leave space for clear button */
padding-left: 0.25em;
padding-top: 2px;
padding-bottom: 2px;
width: 100%;
border-radius: 4px;
border: 1px solid #ccc;
font-size: 16px;
box-sizing: border-box;
transition: border-color 0.2s ease;
}
.clear-btn {
position: absolute;
right: 22px; /* Adjust positioning */
fill: var(--vs-controls-color);
background-color: transparent;
border: 0;
font-size: 1.2em;
color: #333;
cursor: pointer;
z-index: 2; /* Ensure it is above the input */
}
.button:active:hover,
.clear-btn:active:hover {
background-color: #d4d4d4;
border-color: #8c8c8c;
}
/* Chevron buttons container */
.chevron-buttons {
display: flex;
flex-direction: column;
position: absolute;
right: 0px;
top: 0;
bottom: 0;
width: 16px;
padding: 0px 5px 0px 2px;
justify-content: center;
z-index: 2;
}
/* Chevron button styles */
.increment-btn,
.decrement-btn {
background-color: transparent;
border: 0px solid #ccc;
font-size: 10px;
padding: 0px;
cursor: pointer;
color: rgba(60, 60, 60, 0.5);
border-radius: 2px;
}
.increment-btn:hover,
.decrement-btn:hover {
background-color: #ddd;
}
/* Hide the native increment/decrement buttons */
input[type="number"]::-webkit-inner-spin-button,
input[type="number"]::-webkit-outer-spin-button {
-webkit-appearance: none;
margin: 0;
}
input[type="number"] {
-moz-appearance: textfield; /* For Firefox */
}
.numeric-input:focus,
.numeric-input:hover {
border-color: #007bff; /* Match focus color of v-select */
outline: none;
}
.dialog.alert
fieldset:not(.bg-danger):not(.bg-warning):not(.bg-info):not(
.bg-success

View File

@ -3,10 +3,20 @@
:submitAction="deleteTrigger"
:formTitle="
initialized
? `Confirm deletion of Trigger ${triggerNumber} for ${handleContext(library_id, libraries, 'library_id')}`
: 'Confirm circulation trigger deletion'
? $__x(
'Confirm deletion of trigger {triggerNumber} for {library}',
{
triggerNumber,
library: handleContext(
library_id,
libraries,
'library_id'
),
}
)
: $__('Confirm circulation trigger deletion')
"
buttonText="Confirm deletion"
:buttonText="$__('Confirm deletion')"
:disabled="deletionWouldLeaveRuleSetWithoutDelay"
>
<fieldset class="rows">
@ -226,98 +236,6 @@ form ol li {
float: none;
}
.numeric-input-wrapper {
position: relative;
display: inline-block;
width: 30%;
}
.input-with-clear {
position: relative;
display: flex;
align-items: center;
width: 100%;
}
.numeric-input {
padding-right: 40px; /* Adjust to leave space for clear button */
padding-left: 0.25em;
padding-top: 2px;
padding-bottom: 2px;
width: 100%;
border-radius: 4px;
border: 1px solid #ccc;
font-size: 16px;
box-sizing: border-box;
transition: border-color 0.2s ease;
}
.clear-btn {
position: absolute;
right: 22px; /* Adjust positioning */
fill: var(--vs-controls-color);
background-color: transparent;
border: 0;
font-size: 1.2em;
color: #333;
cursor: pointer;
z-index: 2; /* Ensure it is above the input */
}
.button:active:hover,
.clear-btn:active:hover {
background-color: #d4d4d4;
border-color: #8c8c8c;
}
/* Chevron buttons container */
.chevron-buttons {
display: flex;
flex-direction: column;
position: absolute;
right: 0px;
top: 0;
bottom: 0;
width: 16px;
padding: 0px 5px 0px 2px;
justify-content: center;
z-index: 2;
}
/* Chevron button styles */
.increment-btn,
.decrement-btn {
background-color: transparent;
border: 0px solid #ccc;
font-size: 10px;
padding: 0px;
cursor: pointer;
color: rgba(60, 60, 60, 0.5);
border-radius: 2px;
}
.increment-btn:hover,
.decrement-btn:hover {
background-color: #ddd;
}
/* Hide the native increment/decrement buttons */
input[type="number"]::-webkit-inner-spin-button,
input[type="number"]::-webkit-outer-spin-button {
-webkit-appearance: none;
margin: 0;
}
input[type="number"] {
-moz-appearance: textfield; /* For Firefox */
}
.numeric-input:focus,
.numeric-input:hover {
border-color: #007bff; /* Match focus color of v-select */
outline: none;
}
.dialog.alert
fieldset:not(.bg-danger):not(.bg-warning):not(.bg-info):not(
.bg-success

View File

@ -135,8 +135,10 @@
</div>
<div class="page-section" v-if="filtersInitialized">
<legend>
Filter by
<span style="color: blue; font-weight: bold">context</span>
{{ $__("Filter by") }}
<span style="color: blue; font-weight: bold">{{
$__("context")
}}</span>
</legend>
<p v-if="!canManageAnyLibrary" class="alert alert-info">
{{
@ -167,7 +169,7 @@
: libraries.filter(
lib =>
lib.library_id ===
logged_in_library_id
user_library_id
)
"
@update:modelValue="
@ -175,7 +177,9 @@
"
:clearable="false"
:disabled="!canManageAnyLibrary"
placeholder="Default rules for all libraries"
:placeholder="
$__('Default rules for all libraries')
"
>
<template #search="{ attributes, events }">
<input
@ -197,7 +201,7 @@
@update:modelValue="
filterRuleSetsbySearchParam()
"
placeholder="any"
:placeholder="$__('any')"
>
<template #search="{ attributes, events }">
<input
@ -219,7 +223,7 @@
@update:modelValue="
filterRuleSetsbySearchParam()
"
placeholder="any"
:placeholder="$__('any')"
>
<template #search="{ attributes, events }">
<input
@ -277,7 +281,7 @@
"
@click="changeTabContent"
:data-content="`Notice ${number}`"
>{{ $__("Trigger") + " " + number }}
>{{ $__x("Trigger {number}", { number }) }}
</a>
</li>
</ul>
@ -385,7 +389,7 @@ export default {
storeInitialized,
metaInitialized,
canManageAnyLibrary,
logged_in_library_id,
user_library_id,
} = storeToRefs(circRulesStore);
return {
@ -412,7 +416,7 @@ export default {
metaInitialized,
from_branch,
canManageAnyLibrary,
logged_in_library_id,
user_library_id,
scrollToElementById,
};
},

View File

@ -25,9 +25,7 @@
canManageAnyLibrary
? libraries
: libraries.filter(
lib =>
lib.library_id ===
logged_in_library_id
lib => lib.library_id === user_library_id
)
"
:disabled="!isConfirming || !canManageAnyLibrary"
@ -127,7 +125,7 @@ export default {
editMode: { type: [String, Boolean], required: true },
canManageAnyLibrary: { type: Boolean, required: true },
libraries: { type: Array, required: true },
logged_in_library_id: { type: [String, null], default: null },
user_library_id: { type: [String, null], default: null },
patronCategories: { type: Array, required: true },
itemTypes: { type: Array, required: true },
},

View File

@ -5,10 +5,10 @@
id="trigger-edit-form-general-section"
>
<legend v-if="editMode === 'add'">
{{ $__("Add new trigger") + " " + triggerNumber }}
{{ $__x("Add new trigger {triggerNumber}", { triggerNumber }) }}
</legend>
<legend v-else>
{{ $__("Edit trigger") }} {{ " " + triggerNumber }}
{{ $__x("Edit trigger {triggerNumber}", { triggerNumber }) }}
</legend>
<div class="page-section bg-info">
<p>
@ -52,18 +52,11 @@
] !== undefined
"
type="button"
class="clear-btn"
class="btn btn-link clear-btn"
:title="$__('Undo override and reset to fallback')"
@click="handleSetDelayToNull"
>
<svg
xmlns="http://www.w3.org/2000/svg"
width="10"
height="10"
>
<path
d="M6.895455 5l2.842897-2.842898c.348864-.348863.348864-.914488 0-1.263636L9.106534.261648c-.348864-.348864-.914489-.348864-1.263636 0L5 3.104545 2.157102.261648c-.348863-.348864-.914488-.348864-1.263636 0L.261648.893466c-.348864.348864-.348864.914489 0 1.263636L3.104545 5 .261648 7.842898c-.348864.348863-.348864.914488 0 1.263636l.631818.631818c.348864.348864.914773.348864 1.263636 0L5 6.895455l2.842898 2.842897c.348863.348864.914772.348864 1.263636 0l.631818-.631818c.348864-.348864.348864-.914489 0-1.263636L6.895455 5z"
></path>
</svg>
<i class="fa-solid fa-xmark"></i>
</button>
<div class="chevron-buttons">
<button
@ -191,22 +184,14 @@ li {
transition: border-color 0.2s ease;
}
/* overlays the input, clear of the chevrons; appearance comes from btn-link,
matching the equivalent button in EditNotice.vue */
.clear-btn {
position: absolute;
right: 22px;
fill: var(--vs-controls-color);
background-color: transparent;
border: 0;
font-size: 1.2em;
color: #333;
cursor: pointer;
z-index: 2;
}
.button:active:hover,
.clear-btn:active:hover {
background-color: #d4d4d4;
border-color: #8c8c8c;
padding: 0;
color: #333;
}
.chevron-buttons {

View File

@ -25,11 +25,12 @@ import Help from "../../Help.vue";
import Dialog from "../../Dialog.vue";
import "vue-select/dist/vue-select.css";
import { inject } from "vue";
import { $__ } from "@koha-vue/i18n";
export default {
setup() {
const circRulesStore = inject("circRulesStore");
circRulesStore.init(default_view, logged_in_library_id).catch(() => {});
circRulesStore.init(default_view, user_library_id).catch(() => {});
// format letters for display as "name (notice code)" in drop downs.
letters
@ -40,7 +41,7 @@ export default {
});
letters.unshift({
name: "No letter",
name: $__("No letter"),
code: "",
branchcode: "",
value: "",
@ -64,20 +65,8 @@ export default {
background-color: #daecfb !important;
}
/* overrides the 30% width set in css/vue.css */
form .v-select {
display: inline-block;
background-color: white;
width: 50%;
}
.v-select,
input:not([type="submit"]):not([type="search"]):not([type="button"]):not(
[type="checkbox"]
):not([type="radio"]),
textarea {
border-color: rgba(60, 60, 60, 0.26);
border-width: 1px;
border-radius: 4px;
min-width: 30%;
}
</style>

View File

@ -33,6 +33,7 @@ const router = createRouter({
routes,
});
import "../../../../css/vue.css";
import App from "../../components/Admin/CirculationTriggers/Main.vue";
import i18n from "../../i18n";

View File

@ -28,13 +28,13 @@ export const useCircRulesStore = defineStore("circRules", () => {
libraries: [],
patronCategories: [],
userPermissions: null,
logged_in_library_id: null,
user_library_id: null,
letters: [],
ruleSuffixes: ["delay", "notice", "mtt", "restrict"],
transportTypes: [
{ code: "email", name: "Email" },
{ code: "sms", name: "SMS" },
{ code: "print", name: "Print" },
{ code: "email", name: $__("Email") },
{ code: "sms", name: $__("SMS") },
{ code: "print", name: $__("Print") },
],
// rule sets
allDefaultLibraryRawRuleSets: [], // source of truth for default library
@ -90,15 +90,15 @@ export const useCircRulesStore = defineStore("circRules", () => {
const actions = {
// controllers
async init(defaultLibraryId = "*", loggedInLibraryId = null) {
store.logged_in_library_id = loggedInLibraryId;
async init(defaultLibraryId = "*", userLibraryId = null) {
store.user_library_id = userLibraryId;
await this.loadUserPermissions();
await this.getItemTypes();
await this.getLibraries();
await this.getPatronCategories();
// If user can only manage their own library, override the default view
if (!canManageAnyLibrary.value && store.logged_in_library_id) {
this.currentLibraryId = store.logged_in_library_id;
if (!canManageAnyLibrary.value && store.user_library_id) {
this.currentLibraryId = store.user_library_id;
} else {
this.currentLibraryId = defaultLibraryId;
}

View File

@ -299,7 +299,7 @@ subtest 'list_rules() tests' => sub {
};
subtest 'set_rules() tests' => sub {
plan tests => 34;
plan tests => 40;
$schema->storage->txn_begin;
@ -362,6 +362,26 @@ subtest 'set_rules() tests' => sub {
}
);
# Triggers patron: can only edit circulation triggers, and only for their own library
my $triggers_patron = $builder->build_object(
{
class => 'Koha::Patrons',
value => { flags => 0, branchcode => $own_branch }
}
);
$triggers_patron->set_password( { password => $password, skip_validation => 1 } );
my $triggers_userid = $triggers_patron->userid;
$builder->build(
{
source => 'UserPermission',
value => {
borrowernumber => $triggers_patron->borrowernumber,
module_bit => 3,
code => 'manage_circ_triggers',
}
}
);
## Authorized user tests
note("Authorized user setting rules");
@ -473,5 +493,25 @@ subtest 'set_rules() tests' => sub {
$t->put_ok( "//$restricted_userid:$password@/api/v1/circulation_rules" => json => $restricted_rules )
->status_is( 200, "Restricted user can set rules for their own library" );
# manage_circ_rules / manage_circ_triggers separation tests
note("Testing manage_circ_rules and manage_circ_triggers separation");
my $trigger_rules = {
context => {
library_id => $own_branch,
patron_category_id => '*',
item_type_id => '*',
},
overdue_1_delay => 5,
};
$t->put_ok( "//$restricted_userid:$password@/api/v1/circulation_rules" => json => $trigger_rules )
->status_is( 403, "manage_circ_rules alone cannot set trigger rules" );
$t->put_ok( "//$triggers_userid:$password@/api/v1/circulation_rules" => json => $trigger_rules )
->status_is( 200, "manage_circ_triggers can set trigger rules" );
$t->put_ok( "//$triggers_userid:$password@/api/v1/circulation_rules" => json => $restricted_rules )
->status_is( 403, "manage_circ_triggers alone cannot set non-trigger rules" );
$schema->storage->txn_rollback;
};