Extend Record Merging to multiple records

[acwhite211]

Extend Record Merging to multiple records to allow for the handling of businesses rules and keeping the whole merging process inside of one ACID transaction.

[acwhite211]

Still working on creating more unit tests, but please go ahead and start testing with front-end integration.
The old url /api/specify/agent/replace/old_model_id/new_model_id/ still exists, the new extended record merging url is /api/specify/agent/replace/new_model_id/ with a json body containing old_model_ids and new_record_data

[acwhite211]

@melton-jason feel free to add more unit tests

[specifysoftware]

This pull request has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/record-merging-in-specify-7/939/11

[melton-jason]

I will need to take a more indepth look at this and add a unit test for when Agents have Group Person records.
For a quick thrice-over, everything looks good, but I do have some questions.

[grantfitzsimmons]

https://kufish51623-agent-merging-extremos.test.specifysystems.org/specify/overlay/merge/Agent/?records=3%2C5%2C6%2C7%2C8

After waiting for a few minutes, it seems that the merging process failed. It showed a red bar at the top of the dialog, but no error was reported to the user.

When opening the console, it seems there were some errors reported there.

Console Log:
kufish51623-agent-merging-extremos.test.specifysystems.org-16842.zip

[melton-jason]

I have also encountered an issue when Agents are associated with Specify Users.

There is a backend business rule which prevents the deletion of agents when associated with specify users.

specify7/specifyweb/businessrules/agent_rules.py

Lines 5 to 16 in 6f103c4

	@orm_signal_handler('pre_delete', 'Agent')
	def agent_delete_blocked_by_related_specifyuser(agent):
	try:
	user = Specifyuser.objects.get(agents=agent)
	except Specifyuser.DoesNotExist:
	return
	raise BusinessRuleException(
	"agent cannot be deleted while associated with a specifyuser",
	{"table" : "Agent",
	"fieldName" : "specifyuser",
	"agentid" : agent.id,
	"specifyuserid": user.id})

This branch tries to delete the old records (lines 512-514 in specifyweb/views.py), which is what caused an issue for me when testing internally:

# Dedupe by deleting the record that is being replaced and updating the old model ID to the new one
    for old_model_id in old_model_ids:
        target_model.objects.get(id=old_model_id).delete()

@grantfitzsimmons
Assuming we hard-code this (we can brainstorm ways to generalize this), how would we want to handle this case?
The best possibility I can identify is to set the relationship from Specifyuser -> Agent to null (i.e. remove the Agent from the Specifyuser).

[maxpatiiuk]

@melton-jason
Three cases:

• None of the agents are connected to any users - no issue
• One of the agents is connected to a user - link the user to the merged agent instead
• More than one agent is assigned to a (same or different) user - quite unlikely. bail out with a nice error message

[melton-jason]

Yes that would be a nice way of going about this particular case.
Some more context on the More than one agent is assigned to a (same or different) user case; I think having an error message in this case would be fine.
This operation is not supported in the UI at all (I do not know if it is supported in Specify 6 or not): you can not assign an agent to more than one user.

We will need to think of ways to generalize cases such as these (if broad generalization can exist to cover all backend deletion Business Rules).
Here are some options we can take a look into:

• Utilize Django Signals
• Extend Deletion Rules
• Behavior based on attributes in the data model

[maxpatiiuk]

This operation is not supported in the UI at all (I do not know if it is supported in Specify 6 or not): you can not assign an agent to more than one user.

I'm afraid you misunderstood my comment. Indeed, you can't assign the same agent to multiple users.
But, the agent merging tool deals with merging multiple agents into one, thus it's entirely possible (though hopefully not likely) that you would try to merge two agents who both are assigned to a user

[maxpatiiuk]

@melton-jason the simplest solution for this issue:
The front-end for agent merging has code for picking which agent should be considered "main"
We should just change it to consider agent connected to a user to be the main one
See the (untested) fix I pushed

[melton-jason]

@melton-jason the simplest solution for this issue: The front-end for agent merging has code for picking which agent should be considered "main" We should just change it to consider agent connected to a user to be the main one See the (untested) fix I pushed

agent_merge_user.mp4

@maxpatiiuk

Unfortunately, this is not a complete fix.
What about the case when more than one Agents are associated to separate Specify Users?
Regardless of what it chooses as the 'main' Agent, there will always be another Agent associated with a Specifyuser that the backend will try and delete.

[melton-jason]

However, this falls under the third More than one agent is assigned to a (same or different) user - quite unlikely. bail out with a nice error message case.
How likely is this in a production database?
I know we said 'unlikely', but is it unlikely enough for us to be 'comfortable' leaving this bug in Agent Merging for now?
One path forward is that we could release Agent Merging with this issue, be transparent about its existence, and work on a more long-term solution.

[maxpatiiuk]

What about the case when more than one Agents are associated to separate Specify Users?

I don't think this is very likely
@grantfitzsimmons what do you think?
And even if this does occur, a business rule error would occur, which the back-end business error handler code has to handle anyway

[grantfitzsimmons]

What about the case when more than one Agents are associated to separate Specify Users?

I don't think this is very likely @grantfitzsimmons what do you think?

It is not very likely at all

[melton-jason]

https://kufish51623-agent-merging-extremos.test.specifysystems.org/specify/overlay/merge/Agent/?records=3%2C5%2C6%2C7%2C8
After waiting for a few minutes, it seems that the merging process failed. It showed a red bar at the top of the dialog, but no error was reported to the user.

When opening the console, it seems there were some errors reported there.

Console Log: kufish51623-agent-merging-extremos.test.specifysystems.org-16842.zip

This is still concerning, and I have been unable to identify the cause of the issue thus far.
Besides the issue I pointed out regarding Specify Users, I have been able to merge Agents without incidence, both on a local instance and for simple Agents (which I always created beforehand) on the kufish_5_16_23 database.

Perhaps the most concerning and perplexing symptom of this issue is how little it gives in terms of information to debug the problem. The only information that can be gleaned directly from the issue is the 405 (Method Not Allowed) response.
Usually this might be caused by a misconfigured url or the wrong request type, but everything seems correct as far as these are concerned.

@maxpatiiuk
Did I make a mistake in structuring the request in 03f11a1 and subsequentially 8d5f8f6?

[maxpatiiuk]

Code looks ok. This could be caused by test panel restarting. That would explain the 421 error
405 would happen in that case because while test panel is restarting, the requests were sent to the nginx container or to test panel container, which doesn't accept POST

[maxpatiiuk]

#3474 (comment) - I really don't like how the error message is formatted in the video

If I remember correctly, this is only happening when in development. And we still don't know why that's happening?

[melton-jason]

#3474 (comment) - I really don't like how the error message is formatted in the video

If I remember correctly, this is only happening when in development. And we still don't know why that's happening?

In my free time in the past two days I have been looking into why this formatting is happening (#3343). I will update the issue shortly with what I have found in the interim.
If getting back to the correct Django error messages for development is a priority, I can focus on doing so. Or at the very least, I can devote more time to doing so.

[maxpatiiuk]

You have a ton of other important things!
As long as it's better in production, it's not too bad

[melton-jason]

Code looks ok. This could be caused by test panel restarting. That would explain the 421 error 405 would happen in that case because while test panel is restarting, the requests were sent to the nginx container or to test panel container, which doesn't accept POST

Yes, I first thought it might be an issue with the test panel restarting. However, I have attempted to perform that exact merge multiple times, and every time encountered the same behavior.
It would make sense if the requests were being to NGINX or Test Panel containers, where POST requests are not allowed. If this is the case, then how do we manage other POST requests like creating records? The chances of the test panel being down every time @grantfitzsimmons and I have attempted the merge is unlikely. Moreover, I did not encounter any 502 Gateway Time Out or TypeError: Failed to Fetch errors during these attempted merges.

I have successfully merged a variety of Agents both locally and through the same kufish_5_16_23 database, so the endpoint does work correctly.
I am just baffled by the fact that the error encountered by @grantfitzsimmons in #3474 (review) provides very little helpful information. The 405 error is the only information the application provides. There is no Business Rule, Django, or SQLAlchemy error.
Trying to uncover why that merge went wrong would be like trying to find a needle in every haystack.

There is a lot that could go wrong, and we don't have a lot to go off of.

[melton-jason]

I have identified a case on a local copy of umich-mollusks which exhibited the same behavior as in #3474 (review).
I discovered the issue, and was able to apply a fix.

Here is a video showing two agents that I tried to merge which showed the aforementioned behavior.

agent_merging_error.mp4

Assuming the error was likely due to the references, I investigated the 13 Collecting Events which reference ex Rush.
In the Collecting Events, there were multiple instances where R.C. Rush was a Collector alongside ex Rush.
I removed R.C. Rush from being a Collector in every Collecting Event which also referenced ex Rush.

Then I attempted the merge again, and it worked! 🥳

agents_merged.mp4

Our phantom issue is what appears to be an Integrity Error.

In the context of #3474 (review), there must be cases where some records reference both User, Test C and User, W C

I will need to investigate how the endpoint works currently, and I can work on a fix.

[melton-jason]

@maxpatiiuk
Do we want to merge this back to agent-merging before conflicts get out of hand? 😅
You could argue that this change is within the scope of the pull request, but this pull request is already getting a tad too long.
It has accomplished what it sought out to accomplish: modify the endpoint so agent merging is only a single request, and the needed data (the JSON of the record to save, and an Array of id's from the clones') is contained within the POST body.

[maxpatiiuk]

yes please, let's do that

[melton-jason]

Ok. I'd say we can merge this back to agent-merging once @acwhite211 has gone through the review comments

[acwhite211]

Let's go ahead now and merge this with the agent-merging branch and change the front-end code referencing the old api with the new record-merging api so that we can start testing again. Any further problem solving can be a new pull request.

[maxpatiiuk]

Looks good. Two small refactoring things - can be resolved after merging this

[melton-jason]

Perhaps the most concerning and perplexing symptom of this issue is how little it gives in terms of information to debug the problem. The only information that can be gleaned directly from the issue is the 405 (Method Not Allowed) response.
Usually this might be caused by a misconfigured url or the wrong request type, but everything seems correct as far as these are concerned.

#3474 (comment)

This was caused because the backend was returning an http.HttpResponseNotAllowed()

# Determine which of the records will be assigned as old and new with the timestampcreated field
foreign_record_lst = foreign_model.objects.filter(**{field_name_id: new_model_id})
if foreign_record_lst.count() > 1:
    # NOTE: Maybe try handling multiple possible row that are potentially causes the conflict.
    # Would have to go through all constraints and check records based on columns in each constraint. 
    return http.HttpResponseNotAllowed('Error! Multiple records violating uniqueness constraints in ' + table_name) 

Raising an error instead of returning the HttpResponseNotAllowed would make the frontend and toasts behave normally

[maxpatiiuk]

@melton-jason I don't think back-end is at fault here. It's front-end's fault for not handling the error correctly.

There is code that tries to figure out if the error was generated by a permission system (thus display the permission error dialog) or by session time out (thus, prompt the user to sign in instead). It's concerning that this error didn't trigger either error dialog.

Relevant code:

specify7/specifyweb/frontend/js_src/lib/components/Errors/FormatError.tsx

Lines 153 to 177 in bd2ba16

	const permissionError = error as {
	readonly type: 'permissionDenied';
	readonly responseText: string;
	};
	const isPermissionError =
	typeof permissionError === 'object' &&
	permissionError?.type === 'permissionDenied';
	if (isPermissionError) {
	const parsed = formatPermissionsError(
	permissionError.responseText,
	response.url
	);
	if (Array.isArray(parsed)) {
	const [errorObject, errorMessage] = parsed;
	displayError(({ onClose: handleClose }) => (
	<PermissionError error={errorObject} onClose={handleClose} />
	));
	const error = new Error(errorMessage);
	Object.defineProperty(error, 'handledBy', {
	value: handleAjaxError,
	});
	throw error;
	}
	}
	}

Can you please take a look at what's going on there?