Same as bug 34303 and bug 33743, our test suite must parse/process files
that are part of the index, not all the other files.
It's currently failing on kohadev-db-request.txt that is owned by root.
But it's not part of the Koha codebase, this file should not be tested.
There were also node_modules and po files that were processed.
Test plan:
prove t/00-merge-conflict-markers.t
must now return gren
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
All was not returning all the files present in the git index, but all
Koha core files. Let's rename it with 'core' to remove ambiguity.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch refactors the edititem() method in the Standard ILL backend
to use the modern add_or_update_attributes() method instead of raw SQL
delete-all-and-recreate operations.
Problems with the old approach:
- 40+ lines of raw SQL bypassing ORM validation
- Inefficient delete-all-then-insert pattern
- Complex conditional SQL based on column existence
- Manual transaction handling and error-prone bind parameters
- Difficult to maintain and debug
Benefits of the new approach:
- Single line replaces 40+ lines of raw SQL
- Uses ORM validation and relationships
- Only updates attributes when values actually change
- Automatic backend column handling
- Consistent with other modernized Standard backend methods
- Better performance and maintainability
The refactoring:
- Removes all raw SQL queries (DELETE and INSERT statements)
- Removes manual transaction handling
- Removes conditional column existence checks
- Replaces with single add_or_update_attributes() call
- Maintains identical functionality and behavior
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/ILL/Backend/Standard.t
=> SUCCESS: Tests pass! Refactoring maintains compatibility
3. Test ILL request editing in Standard backend:
- Edit existing ILL request metadata
- Add/modify custom fields
- Verify attributes updated correctly
=> SUCCESS: Same functionality, cleaner implementation
4. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds comprehensive unit tests for the edititem() method in the
Standard ILL backend to establish baseline behavior before refactoring.
The tests cover:
- Form stage (initial load) - displays current attributes for editing
- Commit stage (form submission) - processes updates with validation
- Core field updates (title, author, year, etc.)
- Custom field handling (null-delimited key/value pairs)
- Attribute persistence after updates
These tests demonstrate the current raw SQL implementation behavior
and will ensure the upcoming refactoring maintains compatibility.
Current behavior tested:
- edititem() uses multi-stage workflow (init -> form -> commit)
- Validation requires 'type' and 'branchcode' fields
- Custom fields use null-delimited strings for multiple values
- Method returns 'create' (not 'edititem') in commit stage
- Raw SQL delete-all-and-recreate pattern for attribute updates
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/ILL/Backend/Standard.t
=> SUCCESS: Tests pass! Current edititem behavior documented
3. Tests establish baseline for upcoming refactoring
4. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch updates the Standard ILL backend to use the new
add_or_update_attributes() method instead of the problematic
extended_attributes() method that only supports adding new
attributes.
Changes:
- Request creation: Use add_or_update_attributes() for setting metadata
- Request migration: Use add_or_update_attributes() for copying attributes
Benefits:
- Eliminates duplicate key errors when updating existing requests
- Cleaner code - removes array-to-hash conversion boilerplate
- More efficient - only updates attributes when values change
- Consistent with new Koha::ILL::Request API patterns
Locations updated:
- add_request(): Request creation with metadata (line ~1017)
- migrate(): Request migration copying attributes (line ~731)
Test plan:
1. Apply patch
2. Test ILL request creation in Standard backend:
- Create new ILL request with metadata
=> SUCCESS: Request created with attributes
3. Test ILL request migration:
- Migrate existing request to new request
=> SUCCESS: Attributes copied correctly
4. Test updating existing request metadata:
- Edit request and update metadata fields
=> SUCCESS: No duplicate key errors
5. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch simplifies the set_copyright_clearance_confirmed() method
to use the new add_or_update_attributes() method instead of custom
update/create logic.
Benefits:
- Reduces code complexity from 20+ lines to 6 lines
- Eliminates duplicate update/create logic
- Uses the new standardized method for attribute management
- Maintains all existing functionality and test coverage
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/ILL/Request.t
=> SUCCESS: Tests pass! Copyright clearance methods work with new implementation
3. Test copyright clearance workflow still works in ILL module
4. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Currently, ILL backends and plugins must implement custom logic to update
existing extended attributes on ILL requests, as the core extended_attributes()
method only supports adding new attributes and throws duplicate key errors when
attempting to update existing ones. This has led to code duplication across
multiple ILL plugins (Rapido, INNReach, SLNP) that all implement identical
add_or_update_attributes() methods, and forces the Standard backend to use
error-prone patterns when managing request metadata.
This patch adds add_or_update_attributes() method to Koha::ILL::Request that
provides a safe, efficient way to add new attributes or update existing ones
in a single operation, eliminating duplicate key errors and reducing code
duplication.
The method:
- Accepts a hashref of attribute type => value pairs
- Updates existing attributes only when values actually change
- Creates new attributes when they don't exist
- Skips undefined or empty values to avoid constraint errors
- Uses transactions for atomicity
- Returns the request object for method chaining
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/ILL/Request.t
=> SUCCESS: Tests pass! New method works correctly
3. Test adding new attributes works
4. Test updating existing attributes works
5. Test mixed add/update operations work
6. Test empty/undefined values are skipped
7. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch refactors the copyright clearance confirmation functionality
to follow proper separation of concerns by moving business logic from
the controller to the model.
Changes:
- Add set_copyright_clearance_confirmed() method to Koha::ILL::Request
- Add get_copyright_clearance_confirmed() method to Koha::ILL::Request
- Update controller to use dedicated method instead of direct attribute manipulation
- Update staff template to use the new dedicated accessor method
- Add comprehensive test coverage for both methods
Benefits:
- Improved separation of concerns (business logic in model)
- Consistency with existing patterns (get_type, get_type_disclaimer_value)
- Better reusability and testability
- Centralized attribute name management
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/ILL/Request.t
=> SUCCESS: Tests pass! New methods work correctly
3. Test ILL copyright clearance workflow still works:
- Enable ILLModule
- Create HTML customization for ILLModuleCopyrightClearance
- Create OPAC ILL request with copyright clearance
- Verify confirmation is saved and displayed in staff interface
4. Sign off :-D
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
PA amended: Test file name in commit message
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch fixes the copyrightclearance_confirmed not saving if submitting an unauthenticated ILL request.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Test plan, k-t-d:
1) Enable ILLModule
2) Add a new HTML customization, visit:
<staff_url>/cgi-bin/koha/tools/additional-contents.pl?op=add_form&category=html_customizations&editmode=wysiwyg
3) Pick ILLModuleCopyrightClearance as the 'Display location'
4) Pick a 'Publication date' in the past
5) Add a title and content. Save.
6) Create a new ILL request in the OPAC, visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
7) Notice the copyright clearance step is shown. Click 'Yes'.
8) Enter a 'type' and (optionally) some metadata. Click 'Create'.
9) Go back to the module in Staff UI and pick the request just created through the OPAC, visit:
<staff_url>/cgi-bin/koha/ill/ill-requests.pl
10) Notice the message 'Patron has confirmed copyright clearance for this request' is shown next to the patron in the the request details
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
If an item is on order, it may take a while before becoming available.
Showing that too on the opac-user page is helpful for the user and
may reduce questions.
Test plan:
Pick an item and set notforloan to On order (-1) or another custom
negative notforloan status.
Put an item level hold on that item.
Go to the OPAC user page of that patron. Verify that the AV description
is added now to the status column of the holds table.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
To test:
1. Have some holds
2. Go to reserve/request.pl?biblionumber=x for your biblionumber
3. Change at least one hold priority to 'del'
4. Notice the toolbar above the table with Update Hold, Cancel selected, Cancellation reason now looks wonky.
5. APPLY PATCH
6. Try again, the display of the toolbar should be better.
Signed-off-by: Eric Garcia <cubingguy714@gmail.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Odd number of elements in anonymous hash at /kohadevbox/koha/serials/acqui-search-result.pl line 84.
it's coming from closedate that is empty, and output_pref that will
reach this return:
278 if ($str) {
279 local $@;
280 $dt = eval { dt_from_string($str) };
281 Koha::Exceptions::WrongParameter->throw("Invalid date '$str' passed to output_pref") if $@;
282 }
283
284 return if !defined $dt; # NULL date
if str is undef, we don't enter if l.278 and return l.284
which will shift the elements of the hash (because of the list context)
We force the scalar and we are good.
There was no side-effect because closedate was the last of the list.
Note that total (and the 'count(*)' key) is not used in the template.
Test plan:
Create an order for a basket of the first vendor "My vendor" and hit
http://localhost:8081/cgi-bin/koha/serials/acqui-search-result.pl?supplier=d
Without this patch you get the warning
Odd number of elements in anonymous hash at /kohadevbox/koha/serials/acqui-search-result.pl line 84.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The item search table is different from the other tables.
It is using server-side but its own way (not using the REST API).
Test plan:
Search items and filter using the column filters: collections, library,
item types, AVs, etc.
QA: I am not very happy with this patch but, feel free to suggest a
better fix!
Note that "results" was too generic and it felt better to adjust it.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The upgrade DB must be idempotent
if run twice, the second pass fails with
DBI Exception: DBD::mysql::db do failed: Duplicate entry 'ElasticsearchPreventAutoTruncate' for key 'PRIMARY' at /kohadevbox/koha/C4/Installer.pm line 825
We should not insert the pref if it has been renamed already.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch:
* Removes unused logger object instantiation
* Adds missing Test::NoWarnings to the API tests
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch renames the entire API structure from SFTP-specific to generic
file transport terminology to reflect the polymorphic nature of the system.
API Changes:
- Endpoints: /config/sftp_servers -> /config/file_transports
- Parameters: sftp_server_id -> file_transport_id
- Definitions: sftp_server -> file_transport
- Controller: Config::SFTP::Servers -> Config::File::Transports
File Changes:
- api/v1/swagger/definitions/sftp_server.yaml -> file_transport.yaml
- api/v1/swagger/paths/config_sftp_servers.yaml -> config_file_transports.yaml
- Koha/REST/V1/Config/SFTP/Servers.pm -> Koha/REST/V1/Config/File/Transports.pm
- t/db_dependent/api/v1/sftp_servers.t -> file_transports.t
Updated References:
- swagger.yaml: paths, definitions, parameters, tags
- Template JavaScript: API URLs and variable names
- Controller: package name, variable names, error messages
- Tests: endpoints, parameters, descriptions
- API mapping: to_api_mapping field name correction
To test:
1. Apply this patch
2. Run: yarn api:bundle (to regenerate swagger bundle)
3. Run: prove t/db_dependent/api/v1/file_transports.t
4. All API tests should pass
5. Verify admin interface uses new API endpoints
6. Test CRUD operations work correctly
The API now consistently uses 'file_transports' terminology matching
the polymorphic file transport system that supports FTP, SFTP, and
future transport types.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Renamed admin interface files and URLs to reflect the evolution from
'SFTP servers' to 'File transports' functionality:
- admin/sftp_servers.pl -> admin/file_transports.pl
- sftp_servers.tt -> file_transports.tt
- Updated all URL references in templates and admin menu
- Updated body id from admin_sftp_servers to admin_file_transports
The functionality is now future-proof for the addition of new file
transports, so the naming should reflect his broader scope.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds comprehensive tests to verify that Koha::File::Transports
correctly returns polymorphic objects based on the transport field value.
The tests cover:
- build_object() polymorphic creation
- search() with polymorphic resultset iteration
- find() with polymorphic object return
To test:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/File/Transports.t
=> FAIL: Tests fail - objects return base Koha::File::Transport
instead of polymorphic Koha::File::Transport::SFTP/FTP classes
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Resolved circular dependency chain:
Transport.pm → TestTransport.pm → Transports.pm → SFTP.pm → Transport.pm
Changed 'use Koha::BackgroundJob::TestTransport' to runtime 'require'
in the _post_store_trigger method to break the dependency cycle.
This fixes the C3 method resolution order failures in
Koha::File::Transport::FTP and Koha::File::Transport::SFTP.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We now use background jobs triggered by Transport->store to update the
connection status.
Status is now a json blob with more details, so we update the api
definition to reflect that.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Update status field to store longtext so we can sore more details
information about connection statuses from test connection
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Similar to the previous commit, this time we're updating the page
controller to use the new polymorphic classes
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
As the title says, use the new polymorphic classes in the API
controllers
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We add triggers to store in the base class and then use them in the SFTP
transport class to write out the key file to the filesystem for later
use in connect.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds polymorphic transport classes to Koha for FTP and SFTP
file transport configurations.
We introduce a base class for interacting with the base configuration
storage and subclasses to impliment the protocol specific transport
handling.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds _polymorphic_field and _polymorphic_map private methods
to the Koha::File::Transports class to more clearly identify this as a
polymorphic class base for TestBuilder.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
We update the testbuilder tests here to look for signs of a polymorphic
class map and use it to test the different class options for this base
class
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This allows us to do dynamic polymorphic classing on calls to 'find',
'find_or_create', 'single', 'next', 'last' and 'as_list' etc.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Tomás Cohen Arazi
Jonathan Druart
Pedro Amorim
Marcel de Rooy
Lucas Gass
Martin Renvoize
Martin Renvoize