6648 share compare

[sixlighthouses]

What this PR does

Fixes #6648
Added a check in SplitItemReference to see if we have a previousTarget [BaseModel] that has the same id as its source model. If it does return it

Test me

The following link was created using the steps outlined in the ticket

Test Link

Required result with the 2nd dataset being the Copy with the changed date

[steve9164]

What does this check guarantee?

Also, it's probably not a standard use-case of SplitItemReference, but if this.splitSourceItemId changes between calls to loadReference this change will return the old model.

[sixlighthouses]

This check is needed to prevent the second item (dragged from first to second in workbench) from stripping out the required dereferenced details, which is the cause of the bug. i.e. if there is a previousTarget passed in and the id's match return the previousTarget

[steve9164]

It's better to note the bug that was fixed rather than a specific case of it (although it can be good list some specific use cases in a sub-list under the main bug fix entry). Sorry, I don't have a suggestion of a better changelog entry until I've understood the code change in SplitItemReference.ts.

[AnaBelgun]

@steve9164 to talk to @sixlighthouses to clarify the approach

[steve9164]

@na9da to investigate solution to write more documentation and deal with some corner cases (potentially including id of reference changing).

[na9da]

@steve9164 - in the init cycle we call loadReference() in two places - 1 here, and then 2 here. The first call will definitely trigger forceLoadReference(). The second call will trigger forceLoadReference() only if the copy item comes after the source item in the workbench (because mobx thinks the first item has changed so it triggers a recompute).

After the first call, we have a bit of code that explicitly sets the user-stratum for the target item of the reference. This ensures that the copy item has the correct date and whatever changes user made to it:

terriajs/lib/Models/Terria.ts

Lines 1472 to 1482 in 92da14a

	if (isDefined(loadedModel.target)) {
	updateModelFromJson(
	loadedModel.target,
	stratumId,
	dereferenced || {},
	replaceStratum
	).pushErrorTo(
	errors,
	`Failed to update model from JSON: ${loadedModel.target!.uniqueId}`
	);
	}

However, if the 2nd call to loadReference() results in a forceLoadReference() call, then it will again create a new target. But after this new target is created, the user-stratum is not updated which causes the bug.

What the change in this PR does is to avoid creating a new target if the source item has not changed, which fixes it for SplitItemReference

But I think this bug might also affect other references (?). Maybe not, because the other references do not refer to a workbench item.

What are other possible solutions here? Do we avoid the second call to loadReference()?