[bug] Automatic movement record not created when a storage location is moved

[poplarmedia]

I am using a MacOS Homebrew as a dev environment with Apache, MariaDB, and PHP 8.3. The codebase is Providence dev/2.0 branch.

I am following the documentation here:
https://docs.collectiveaccess.org/providence/user/workflow/history_tracking_current_value

I have set record_movement_information_when_moving_storage_location = 1 and successfully tested a current_location history-tracking policy in ...conf/local/app.conf. When I move a storage location like a box or a cabinet from one room to another, it does not create movement record, and the move is not tracked in the logs of the objects. The box moves successfully, the objects in the box move correctly, the objects move inside to the new location as well, but nothing is updated beside the location:

• Time stamp on the object does not change from last manual update,
• The object log does not show the change in location.
• The storage container log updates with a geo-reference change, no other details.
• History tracking – chronology does work to move an object, and shows it in the log
• Move is not generated, is not findable in search

I did some testing and I think its is in ca_storage_locations::saveBundlesForScreen(), Where the movement tracking runs, $old_parent_id and $new_parent_id are equal on every move, so the guard that skips tracking when there is "no actual movement" fires every time and tracking is never reached.

See this line of code in app/models/ca_storage_locations.php

if ($old_parent_id === $new_parent_id) { return $rc; } // don't track if there's no actual movement

I added some logging and got this result on several move attempts

SBFS-DECIDE rc=true pc=true old=8   new=8
SBFS-DECIDE rc=true pc=true old=177 new=177
SBFS-DECIDE rc=true pc=true old=22  new=22

Steps to reproduce

1. Configure current-location history tracking with a ca_movements policy element
2. Open a storage location with objects inside
3. Using the hierarchy_location bundle, go to the Move tab to select a different parent location.
4. Movement form appears below
5. Fill in the Movement details form and save.

[poplarmedia]

Including ca_storage_locations.php file with logging code

ca_storage_locations.php

added.

[poplarmedia]

Also the history tracking, inspector, and movement configs from local/app.conf

ca_movements_disable = 0
ca_movements_lookup_delimiter = /
ca_movements_breakout_find_by_type_in_menu = 1
ca_movements_no_new_submenu = 1


# -----------------------------------
# History tracking policies
# -----------------------------------

history_tracking_policies = {
	defaults = {
				ca_objects = current_location
		},
		policies = {
				current_location = {
						name = _(Current location),
						table = ca_objects,
						contents = {
								expandHierarchically = 1,
								template = "^ca_object_representations.media.icon ^ca_objects.preferred_labels"
						},
						elements = {
								ca_storage_locations = {
										__default__ = {
												date = ca_objects_x_storage_locations.effective_date,
												setInterstitialElementsOnAdd = [effective_date],
												useDatePicker = 0,
												template = "<l>^ca_storage_locations.hierarchy.preferred_labels.name%delimiter=_/_</l>  <ifdef code='ca_objects_x_storage_locations.movement_by'> <br>MOVED BY: ^ca_objects_x_storage_locations.movement_by</ifdef>  <ifdef code='ca_objects_x_storage_locations.movement_comments'> <br>COMMENTS: ^ca_objects_x_storage_locations.movement_comments</ifdef>",
												trackingRelationshipType = related,
												restrictToRelationshipTypes = [related]
										}
								},
								ca_occurrences = {
										exhibition = {
												date = ca_occurrences.exhibition_date,
												setInterstitialElementsOnAdd = [effective_date],
												template = "<l>^ca_occurrences.preferred_labels.name</l>",
										},
										__default__ = {
												date = ca_objects_x_occurrences.effective_date,
												setInterstitialElementsOnAdd = [effective_date],
												template = "<l>^ca_occurrences.idno</l> ^ca_occurrences.preferred_labels.name",
										}
								 },
								ca_loans = {
										__default__  = {
												date = ca_loans_x_objects.effective_date,
												setInterstitialElementsOnAdd = [effective_date],
												color = F78B8B,
												template = <l>^ca_loans.idno</l> ^ca_loans.preferred_labels (^ca_loans.institution ^ca_loans.date) <ifdef code='ca_loans_x_objects.movement_comments'> <br>COMMENTS: ^ca_loans_x_objects.movement_comments</ifdef>,
												restrictToRelationshipTypes = [loan]
										}
								},
								ca_movements = {
									__default__ = {
												date = ca_movements.removal_date,
												trackingRelationshipType = related,
												template = "<unit relativeTo='ca_movements'>[^ca_movements.idno] <l>^ca_movements.preferred_labels.name</l><ifdef code='ca_movements.movement_reason'>, ^ca_movements.movement_reason</ifdef><br/>
												<unit relativeTo='ca_storage_locations' restrictToRelationshipTypes='related'>Moved to <l>^ca_storage_locations.hierarchy.preferred_labels.name%delimiter=_/_</l></unit>
												</unit>",
												useRelated = ca_storage_locations,
												useRelatedRelationshipType = related,
												originalLocationTrackingRelationshipType = original_location,
												newLocationTrackingRelationshipType = new_location,
												subLocationTrackingRelationshipType = sub_location
										}
								}
							}
						}
				}
}

# -----------------------------------
# Inspector display
# -----------------------------------

inspector_tracking_displays = {
		 ca_objects = {
				 __default__ = {
						 policy = current_location,
						 label = _(Current location)
				 }
		 }
}

# -----------------------------------
# Additional Movements Fuctionality
# -----------------------------------

record_movement_information_when_moving_storage_location = 1
original_storage_location_path_template = ^ca_storage_locations.hierarchy.preferred_labels.name%delimiter=_ /_

[Monica-Wood]

Have you been able to successfully create a movement record manually from an object record?

I believe when you move an storage location in the hierarchy (eg. from one shelf to another), it should bring up a movement record to edit and save.

Make sure all the relationship types you have referred to exist correctly in your system and the movement, storage location and object records have the metadata referred to.

You might also need to set movement_storage_location_date_element in app.conf

[poplarmedia]

Have you been able to successfully create a movement record manually from an object record?

I can create a movement record and relate an object to it. The object record then displays a move record in the location chronology bundle, and there is no related new storage location that was the destination in the move

I believe when you move an storage location in the hierarchy (eg. from one shelf to another), it should bring up a movement record to edit and save.

Yes it does this, I populate the form, it processes the move and no new movement record is created. there is a new log entry for the container, but it does not record the move, just blanks to any empty fields. No log entry on the objects in the container. The objects are assigned the new location, but retain the timestamp of the last time they were associated with the container

Make sure all the relationship types you have referred to exist correctly in your system and the movement, storage location and object records have the metadata referred to.

confirmed

You might also need to set movement_storage_location_date_element in app.conf

Missed that one, save the setting, re-ran _caUtils reload-current-values-for-history-tracking-policies_, with no change in results.

[Monica-Wood]

Can you please list all of the relationships types you have set up in the system for the following relationships:

Movement <> Object
Movement <> Storage Location
Object <> Storage Location

Thanks

[Monica-Wood]

Just want to let you know that I have been investigating this issue and can confirm that it is not triggering the movement creation due to the line
if ($old_parent_id === $new_parent_id) { return $rc; } // don't track if there's no actual movement in ca_storage_locations.php function saveBundlesForScreen

both $old_parent_id and $new_parent_id are set with the same value using (int)$this->get('ca_storage_locations.parent_id') so they will always equal the same.

[Monica-Wood]

I am assuming that when the if statement on ln 529 calls
$rc = parent::saveBundlesForScreen($screen, $request, $options)
it should change the context of $this for the storage location, so $new_parent_id = (int)$this->get('ca_storage_locations.parent_id'); should be the new id... but this is not happening.

Has BundleableLabelableBaseModelWithAttributes::saveBundlesForScreen() changed at all in the past year? Or some sort of caching holding onto the old object in $this?

[poplarmedia]

Monica,
Thanks for looking into this issue. Answers to your questions:

Can you please list all of the relationships types you have set up in the system for the following relationships:

Movement <> Object Movement <> Storage Location Object <> Storage Location

Thanks

Cheers,
Matt

---

Movement ↔ Object (ca_movements_x_objects)

type_code	Forward	Reverse	Default
part	has as part of	is part of	Yes

Movement ↔ Storage Location (ca_movements_x_storage_locations)

type_code	Forward	Reverse	Default
related	is related to	is related to	No
new_location	moved to	is new location of	Yes
original_location	moved from	was original location of	No
sub_location	includes sub-location	is sub-location of	No

Object ↔ Storage Location (ca_objects_x_storage_locations)

type_code	Forward	Reverse	Default
related	is located at	is location of	Yes

[poplarmedia]

@SethKaufman any thoughts on Monica's question? Is there another use case you would like us to verify?

[poplarmedia]

I think I found something after reviewing the git history. The $old_parent_id/$new_parent_id bug has been present since 5cdb3074a (PROV-3115). This is not a recent regression — both values have always been read from $this->get('ca_storage_locations.parent_id'), so they are always equal and the guard always fires. The commented-out code that preceded this implementation read from $_REQUEST, which is the correct approach.

Two fixes are required in ca_storage_locations::saveBundlesForScreen():

Fix 1 — Read new parent from request, not model (line ~505):

// Before
$new_parent_id = (int)$this->get('ca_storage_locations.parent_id');

// After
$new_parent_id = (int)$request->getParameter('parent_id', pInteger);

Fix 2 — Resolve movement type code to numeric ID before set():

// Before
if (!$movement_type || !$t_movement->getTypeIDForCode($movement_type)) { $movement_type = $t_movement->getDefaultTypeID(); }

// After
$movement_type = $t_movement->getTypeIDForCode($movement_type) ?: $t_movement->getDefaultTypeID();

The original code left $movement_type as a string code rather than resolving it to a numeric ID, causing set('type_id', $movement_type) to fail validation.

Both fixes have been tested on my dev instance and confirmed to unblock movement creation but there is a blocker in movement form.

After applying both fixes, movement creation is attempted but fails because $movement_form_name is null. The code looks for a *_movement_form_name key in $_REQUEST to identify the embedded movement form attached to the hierarchy_location bundle — but no such parameter is being submitted.

I get this error:

Could not create movement: A valid type must be specified
There are errors preventing ALL information from being saved:
Movement type: A valid type must be specified

Question: How should the embedded movement correctly behave in the hierarchy_location bundle? Is this driven by always_create_new_movement in app.conf, a bundle placement setting, or was this defined in the installation profile?

[Monica-Wood]

I believe something changed to stop the$new_parent_id from being set correctly after $rc was set with saveBundlesForScreen (see my next comment). but this is the change that I made to ensure the $new_parent_id was read from the form being submitted.

in app/models/ca_storage_locations.php
remove line 530

providence/app/models/ca_storage_locations.php

Line 530 in 0e66508

$new_parent_id = (int)$this->get('ca_storage_locations.parent_id');

Add to line 545
$new_parent_id = $request->getParameter($matches[1].'_new_parent_id', pInteger);

Move the comparison check on line 532

providence/app/models/ca_storage_locations.php

Line 532 in 0e66508

if ($old_parent_id === $new_parent_id) { return $rc; } // don't track if there's no actual movement

to line 549 (After the new_parent_id variable is set)

This doesn't fix the invalid movement type_id error, but fixes the old and new parent id issue.

I'll add another reply with my suggested fix to the movement type_id error.

[Monica-Wood]

The issue with the 'Invalid type id' for the movement record.

In app/lib/BundlableLabelableBaseModelWithAttributes.php function saveBundlesForScreen there is a section that overwrites the previously set type_id with the type_id of the current record (that being the ca_storage_locations type).

This happens in 2 places:

providence/app/lib/BundlableLabelableBaseModelWithAttributes.php

Line 4195 in 0e66508

$this->set('type_id', $_REQUEST['type_id']);

and

providence/app/lib/BundlableLabelableBaseModelWithAttributes.php

Line 4212 in 0e66508

$this->set($vs_f, $vs_v);

These bits of code haven't really changed much in recent times, so while they are partly to blame, I think the main culprit is the insert function that is now called in a different order inside saveBundlesForScreen after the commit:

dc44302#diff-bcfc762b82dd6ff893061bb9cce06ae188e000a9cb2215ff4d760d5869b83902L4289

I installed fresh from version 2.0.9 which is before this change and movements worked perfectly with no changes (not even the parent_id above).

Assuming this reordering of the insert command is necessary, the places where the type_id is set in the lines above needs to have a check in place to not overwrite that value if already set by checking $this->get('type_id')

@collectiveaccess: Do you think this is on the right track for fixing this? I can submit a pull request if so.

[poplarmedia]

After reviewing @Monica-Wood's research, I tested it against a clean dev/2.0 checkout--removed my hacks. The diagnosis on $new_parent_id is correct, and tracing the resulting type_id error surfaced a second, related defect in the base model's field-handling loop. Both are confirmed fixed on my local dev. I have these patches below for the two files she mentions.

If you want, I can do a commit to dev/2.0

app/models/ca_storage_locations.php

Old $new_parent_id assignment ~line 530, old guard check ~line 532, $movement_form_name init ~line 540, insertion point for new $new_parent_id read ~line 545, moved guard check placed just above ~line 550.

Before:

if (($rc = parent::saveBundlesForScreen($screen, $request, $options)) && $parent_changed) {
    $new_parent_id = (int)$this->get('ca_storage_locations.parent_id');

    if ($old_parent_id === $new_parent_id) { return $rc; }	// don't track if there's no actual movement

    unset($options['ui_instance']);

    // Get list of policies that involve movements – we'll  need to generate movement records for these policies if they are so configured.
    if (!is_array($policies = ca_movements::getDependentHistoryTrackingCurrentValuePolicies('ca_movements'))) { return $rc; }

    // Look for incoming movement form attached to hierarcy_location bundle
    $movement_form_name = $movemenr_form_screen = null;
    foreach($_REQUEST as $key => $val) {
        if (preg_match('!^(.*)_movement_form_name$!', $key, $matches)) {
            $movement_form_name = $request->getParameter($matches[1].'_movement_form_name', pString);
            $movement_form_screen = $request->getParameter($matches[1].'_movement_screen', pString);

            break;
        }
    }

    if ($movement_form_name && $movement_form_screen) {

After:

if (($rc = parent::saveBundlesForScreen($screen, $request, $options)) && $parent_changed) {
    $new_parent_id = $old_parent_id;	// safe default if no movement form is found below

    unset($options['ui_instance']);

    // Get list of policies that involve movements – we'll  need to generate movement records for these policies if they are so configured.
    if (!is_array($policies = ca_movements::getDependentHistoryTrackingCurrentValuePolicies('ca_movements'))) { return $rc; }

    // Look for incoming movement form attached to hierarcy_location bundle
    $movement_form_name = $movemenr_form_screen = null;
    foreach($_REQUEST as $key => $val) {
        if (preg_match('!^(.*)_movement_form_name$!', $key, $matches)) {
            $movement_form_name = $request->getParameter($matches[1].'_movement_form_name', pString);
            $movement_form_screen = $request->getParameter($matches[1].'_movement_screen', pString);
            $new_parent_id = $request->getParameter($matches[1].'_new_parent_id', pInteger);

            break;
        }
    }

    if ($old_parent_id === $new_parent_id) { return $rc; }	// don't track if there's no actual movement

    if ($movement_form_name && $movement_form_screen) {

app/lib/BundlableLabelableBaseModelWithAttributes.php

Ballpark line numbers: Edit 1 ~lines 4194-4195, Edit 2 ~line 4211.

Edit 1 — before:

if (isset($va_fields_by_type['intrinsic']['mandatory_type_id'])) {
    $this->set('type_id', $_REQUEST['type_id']);
}

Edit 1 — after:

if (isset($va_fields_by_type['intrinsic']['mandatory_type_id']) && !$this->get('type_id')) {
    $this->set('type_id', $_REQUEST['type_id']);
}

Edit 2 — before:

if(!is_null($vs_v)) { 
    $this->set($vs_f, $vs_v);
}

Edit 2 — after:

if(($vs_f === 'type_id') && $this->get('type_id')) {
    // already set programmatically (e.g. auto-created movement) — don't overwrite
} elseif(!is_null($vs_v)) { 
    $this->set($vs_f, $vs_v);
}

[poplarmedia]

Hey, Can I get a response to the issue and the PR with the proposed fix?

[collectiveaccess]

The code you're changing is complicated and handles many more use cases than the one you're patching for. We will review your proposed changes when time allows.