From 0a8af2f96096bced8f199f80d21176289dd720d3 Mon Sep 17 00:00:00 2001 From: alec_dev Date: Tue, 9 May 2023 15:54:11 -0500 Subject: [PATCH 01/10] extend record merging with multiple records --- specifyweb/specify/api_tests.py | 90 ++++++++++++++++++++++ specifyweb/specify/urls.py | 1 + specifyweb/specify/views.py | 130 +++++++++++++++++++++++++++++--- 3 files changed, 212 insertions(+), 9 deletions(-) diff --git a/specifyweb/specify/api_tests.py b/specifyweb/specify/api_tests.py index 2c2f9851313..9a14238fa22 100644 --- a/specifyweb/specify/api_tests.py +++ b/specifyweb/specify/api_tests.py @@ -696,3 +696,93 @@ def test_agent_address_replacement(self): # Assert there is only one address the points to agent_1 self.assertEqual(models.Address.objects.filter(agent_id=7).count(), 1) self.assertEqual(models.Address.objects.filter(agent_id=6).exists(), False) + + def test_agent_address_multiple_replacement(self): + c = Client() + c.force_login(self.specifyuser) + + # Create agents and a collector relationship + agent_1 = models.Agent.objects.create( + id=7, + agenttype=0, + firstname="agent", + lastname="007", + specifyuser=None) + agent_2 = models.Agent.objects.create( + id=6, + agenttype=0, + firstname="agent", + lastname="006", + specifyuser=None) + agent_3 = models.Agent.objects.create( + id=5, + agenttype=0, + firstname="agent", + lastname="005", + specifyuser=None) + + # Create mock addresses + models.Address.objects.create( + id=1, + timestampcreated="22022-11-30 14:34:51.000", + address="1234 Main St.", + agent=agent_1 + ) + models.Address.objects.create( + id=2, + timestampcreated="2022-11-30 14:33:30.000", + address="5678 Rainbow Rd.", + agent=agent_2 + ) + models.Address.objects.create( + id=3, + timestampcreated="2022-11-30 14:32:30.000", + address="2468 Mass St.", + agent=agent_3 + ) + + # Assert that the api request ran successfully + response = c.post( + f'/api/specify/agent/replace/{agent_1.id}/', + data=json.dumps({ + 'collection_id': self.collection.id, + 'old_record_ids': [ + agent_2.id, + agent_3.id + ], + 'new_record_data': { + 'addresses': [ + { + 'address': '1234 Main St.', + 'timestampcreated': '22022-11-30 14:34:51.000', + 'agent': agent_1.id + }, + { + 'address': '5678 Rainbow Rd.', + 'timestampcreated': '2022-11-30 14:33:30.000', + 'agent': agent_1.id + }, + { + 'address': '2468 Mass St.', + 'timestampcreated': '2022-11-30 14:32:30.000', + 'agent': agent_1.id + }, + { + 'address': '1345 Jayhawk Blvd.', + 'timestampcreated': '22022-11-30 14:34:51.000', + 'agent': agent_1.id + } + ], + 'jobtitle': 'shardbearer' + } + }), + content_type='application/json') + self.assertEqual(response.status_code, 204) + + # Assert there is only one address the points to agent_1 + self.assertEqual(models.Address.objects.filter(agent_id=7).count(), 4) + self.assertEqual(models.Address.objects.filter(agent_id=6).exists(), False) + self.assertEqual(models.Address.objects.filter(agent_id=5).exists(), False) + + # Assert that the new_record_data was updated in the db + self.assertEqual(models.Agent.objects.get(id=7).jobtitle, 'shardbearer') \ No newline at end of file diff --git a/specifyweb/specify/urls.py b/specifyweb/specify/urls.py index 3a132e5a249..f34b09387c3 100644 --- a/specifyweb/specify/urls.py +++ b/specifyweb/specify/urls.py @@ -10,6 +10,7 @@ urlpatterns = [ # replace record url(r'^specify/(?P\w+)/replace/(?P\d+)/(?P\d+)/$', views.record_merge), + url(r'^specify/(?P\w+)/replace/(?P\d+)/$', views.record_merge_extended), # the main business data API url(r'^specify_schema/openapi.json$', schema.openapi), diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index fc49dccb954..d7fd547d149 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -6,17 +6,19 @@ import mimetypes from functools import wraps from itertools import groupby +from typing import Any, Dict, List from django import http from django.conf import settings from django.db import IntegrityError, router, transaction, connection, models +from django.db.models import Q from django.db.models.deletion import Collector from django.views.decorators.cache import cache_control from django.views.decorators.http import require_http_methods, require_POST from specifyweb.businessrules.exceptions import BusinessRuleException from specifyweb.permissions.permissions import PermissionTarget, \ - PermissionTargetAction, PermissionsException, check_permission_targets + PermissionTargetAction, PermissionsException, check_permission_targets, table_permissions_checker from . import api, models as spmodels from .specify_jar import specify_jar @@ -404,7 +406,8 @@ class ReplaceRecordPT(PermissionTarget): delete = PermissionTargetAction() @transaction.atomic -def record_merge_fx(model_name: str, old_model_id: int, new_model_id: int) -> http.HttpResponse: +def record_merge_fx(model_name: str, old_model_ids: List[int], new_model_id: int, + new_record_info: Dict[str, Any]=None) -> http.HttpResponse: """Replaces all the foreign keys referencing the old record ID with the new record ID, and deletes the old record. """ @@ -415,10 +418,11 @@ def record_merge_fx(model_name: str, old_model_id: int, new_model_id: int) -> ht return http.HttpResponseNotFound("model_name: " + model_name + "does not exist.") # Check to make sure both the old and new agent IDs exist in the table - if not target_model.objects.filter(id=old_model_id).select_for_update().exists(): - return http.HttpResponseNotFound(model_name + "ID: " + str(old_model_id) + " does not exist.") if not target_model.objects.filter(id=new_model_id).select_for_update().exists(): return http.HttpResponseNotFound(model_name + "ID: " + str(new_model_id) + " does not exist.") + for old_model_id in old_model_ids: + if not target_model.objects.filter(id=old_model_id).select_for_update().exists(): + return http.HttpResponseNotFound(model_name + "ID: " + str(old_model_id) + " does not exist.") # Get dependent fields and objects of the target object target_object = target_model.objects.get(id=old_model_id) @@ -451,7 +455,11 @@ def record_merge_fx(model_name: str, old_model_id: int, new_model_id: int) -> ht continue # Filter the objects in the foreign model that references the old target model - foreign_objects = foreign_model.objects.filter(**{field_name_id: old_model_id}) + # foreign_objects = foreign_model.objects.filter(**{field_name_id: old_model_id}) + query: Q = Q(**{field_name_id: old_model_ids[0]}) + for old_model_id in old_model_ids[1:]: + query.add(Q(**{field_name_id: old_model_id}), Q.OR) + foreign_objects = foreign_model.objects.filter(query) # Update and save the foreign model objects with the new_model_id for obj in foreign_objects: @@ -502,9 +510,29 @@ def update_record(record: models.Model): return response # Dedupe by deleting the agent that is being replaced and updating the old model ID to the new one - target_model.objects.get(id=old_model_id).delete() - - return http.HttpResponse('', status=204) + for old_model_id in old_model_ids: + target_model.objects.get(id=old_model_id).delete() + + # Update new record with json info, if given + has_new_record_info: bool = False if new_record_info is None else True + if has_new_record_info: + obj = api.put_resource(new_record_info['collection'], + new_record_info['specify_user'], + model_name, + new_model_id, + new_record_info['version'], + new_record_info['new_record_data']) + + # Return http response + if has_new_record_info: + checker = table_permissions_checker(new_record_info['collection'].id, + new_record_info['specify_user'], + "read") + return http.HttpResponse(api.toJson(api._obj_to_data(obj, checker)), + content_type='application/json', + status=204) + else: + return http.HttpResponse('', status=204) @openapi(schema={ 'post': { @@ -555,5 +583,89 @@ def record_merge(request: http.HttpRequest, model_name: str, old_model_id: int, # if model_name.lower() != 'Agent'.lower(): # return http.HttpResponseForbidden(f'Record Merging for the {model_name} table is not yet been fully tested.') - response = record_merge_fx(model_name, old_model_id, new_model_id) + response = record_merge_fx(model_name, [old_model_id], new_model_id) + return response + +@openapi(schema={ + 'post': { + "requestBody": { + "required": True, + "description": "Rplace a list of old records with a new record.", + "content": { + "application/json": { + "schema": { + "type": "object", + "description": "The request body.", + "properties": { + "model_name": { + "type": "string", + "description": "The name of the table that is to be merged." + }, + "new_model_id": { + "type": "integer", + "description": "The new ID value of the model that is replacing the old one." + }, + "collection_id": { + "type": "integer", + "description": "The collection ID." + }, + "old_record_ids": { + "type": "array", + "items": { + "type": "integer" + }, + "description": "The old record IDs." + }, + "new_record_data": { + "type": "object", + "description": "The new record data." + } + }, + 'required': ['model_name', 'new_model_id', 'collection_id', 'old_record_ids', 'new_record_data'], + 'additionalProperties': False + } + } + } + }, + "responses": { + "204": {"description": "Success",}, + "404": {"description": "The ID specified does not exist."}, + "405": {"description": "A database rule was broken."} + } + }, +}) + +@login_maybe_required +@require_POST +def record_merge_extended(request: http.HttpRequest, model_name: str, new_model_id: int) -> http.HttpResponse: + """Replaces all the foreign keys referencing the old record IDs + with the new record ID, and deletes the old records. + """ + request_params = http.QueryDict(request.META['QUERY_STRING']) + + version = None + try: + version = request_params['version'] + except KeyError: + try: + version = request.META['HTTP_IF_MATCH'] + except KeyError: + version = None + if version is None: + version = getattr(spmodels, model_name.title()).objects.get(id=new_model_id).version + + checker = table_permissions_checker(request.specify_collection, request.specify_user_agent, "read") + check_permission_targets(None, request.specify_user.id, [ReplaceRecordPT.update, ReplaceRecordPT.delete]) + + data = json.loads(request.body) + old_model_ids = data['old_record_ids'] + new_record_info = { + 'agent_id': new_model_id, + 'collection': request.specify_collection, + 'specify_user': request.specify_user_agent, + 'version': version, + 'new_record_data': data['new_record_data'] + } + + response = record_merge_fx(model_name, old_model_ids, new_model_id, new_record_info) return response \ No newline at end of file From 9ecca0a91b5e2eaef88b7e8684347b4dc03c088d Mon Sep 17 00:00:00 2001 From: alec_dev Date: Tue, 9 May 2023 16:14:55 -0500 Subject: [PATCH 02/10] remove collection_id from request --- specifyweb/specify/api_tests.py | 1 - specifyweb/specify/views.py | 4 ---- 2 files changed, 5 deletions(-) diff --git a/specifyweb/specify/api_tests.py b/specifyweb/specify/api_tests.py index 9a14238fa22..d72a4b64138 100644 --- a/specifyweb/specify/api_tests.py +++ b/specifyweb/specify/api_tests.py @@ -745,7 +745,6 @@ def test_agent_address_multiple_replacement(self): response = c.post( f'/api/specify/agent/replace/{agent_1.id}/', data=json.dumps({ - 'collection_id': self.collection.id, 'old_record_ids': [ agent_2.id, agent_3.id diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index d7fd547d149..51110708caa 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -605,10 +605,6 @@ def record_merge(request: http.HttpRequest, model_name: str, old_model_id: int, "type": "integer", "description": "The new ID value of the model that is replacing the old one." }, - "collection_id": { - "type": "integer", - "description": "The collection ID." - }, "old_record_ids": { "type": "array", "items": { From 8d5d86955b89e6435b62ebafa24f36c9ede1d3a0 Mon Sep 17 00:00:00 2001 From: alec_dev Date: Tue, 9 May 2023 16:37:51 -0500 Subject: [PATCH 03/10] fix recursive call to record_merge_fx --- specifyweb/specify/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index 51110708caa..9a81f4f4f6c 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -490,7 +490,7 @@ def record_merge_recur(): x.id)) # Make a recursive call to record_merge to resolve duplication error - response = record_merge_fx(table_name, old_record.pk, new_record.pk) + response = record_merge_fx(table_name, [old_record.pk], new_record.pk) if old_record.pk != obj.pk: update_record(new_record) return response From 5da01d92081ef22e4b90b35dd317e48bfb6c0b58 Mon Sep 17 00:00:00 2001 From: alec_dev Date: Fri, 12 May 2023 11:43:06 -0500 Subject: [PATCH 04/10] remove old api url --- specifyweb/specify/api_tests.py | 38 ++++++++----- specifyweb/specify/urls.py | 3 +- specifyweb/specify/views.py | 96 +++++---------------------------- 3 files changed, 41 insertions(+), 96 deletions(-) diff --git a/specifyweb/specify/api_tests.py b/specifyweb/specify/api_tests.py index d72a4b64138..38ec3c42fde 100644 --- a/specifyweb/specify/api_tests.py +++ b/specifyweb/specify/api_tests.py @@ -576,9 +576,12 @@ def test_replace_agents(self): # Assert that the api request ran successfully response = c.post( - f'/api/specify/agent/replace/{agent_1.id}/{agent_2.id}/', - data=[], - content_type='text/plain') + f'/api/specify/agent/replace/{agent_2.id}/', + data=json.dumps({ + 'old_record_ids': [agent_1.id] + }), + content_type='application/json' + ) self.assertEqual(response.status_code, 204) # Assert that the collector relationship was updated correctly to the new agent @@ -591,9 +594,12 @@ def test_replace_agents(self): # Assert that a new api request will not find the old agent response = c.post( - f'/api/specify/agent/replace/{agent_1.id}/{agent_2.id}/', - data=[], - content_type='text/plain') + f'/api/specify/agent/replace/{agent_2.id}/', + data=json.dumps({ + 'old_record_ids': [agent_1.id] + }), + content_type='application/json' + ) self.assertEqual(response.status_code, 404) def test_record_recursive_merge(self): @@ -641,9 +647,13 @@ def test_record_recursive_merge(self): # Assert that the api request ran successfully response = c.post( - f'/api/specify/agent/replace/{agent_1.id}/{agent_2.id}/', - data=[], - content_type='text/plain') + f'/api/specify/agent/replace/{agent_2.id}/', + data=json.dumps({ + 'old_record_ids': [agent_1.id], + 'new_record_data': None + }), + content_type='application/json' + ) self.assertEqual(response.status_code, 204) # Assert that only one of the Authors remains @@ -688,9 +698,13 @@ def test_agent_address_replacement(self): # Assert that the api request ran successfully response = c.post( - f'/api/specify/agent/replace/{agent_2.id}/{agent_1.id}/', - data=[], - content_type='text/plain') + f'/api/specify/agent/replace/{agent_1.id}/', + data=json.dumps({ + 'old_record_ids': [agent_2.id], + 'new_record_data': None + }), + content_type='application/json' + ) self.assertEqual(response.status_code, 204) # Assert there is only one address the points to agent_1 diff --git a/specifyweb/specify/urls.py b/specifyweb/specify/urls.py index f34b09387c3..7a7beca65ba 100644 --- a/specifyweb/specify/urls.py +++ b/specifyweb/specify/urls.py @@ -9,8 +9,7 @@ urlpatterns = [ # replace record - url(r'^specify/(?P\w+)/replace/(?P\d+)/(?P\d+)/$', views.record_merge), - url(r'^specify/(?P\w+)/replace/(?P\d+)/$', views.record_merge_extended), + url(r'^specify/(?P\w+)/replace/(?P\d+)/$', views.record_merge), # the main business data API url(r'^specify_schema/openapi.json$', schema.openapi), diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index 9a81f4f4f6c..fd2c26db6e1 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -509,13 +509,13 @@ def update_record(record: models.Model): if response is not None and response.status_code != 204: return response - # Dedupe by deleting the agent that is being replaced and updating the old model ID to the new one + # 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() # Update new record with json info, if given - has_new_record_info: bool = False if new_record_info is None else True - if has_new_record_info: + has_new_record_info = False if new_record_info is None else True + if has_new_record_info and 'new_record_data' in new_record_info and new_record_info['new_record_data'] is not None: obj = api.put_resource(new_record_info['collection'], new_record_info['specify_user'], model_name, @@ -524,68 +524,8 @@ def update_record(record: models.Model): new_record_info['new_record_data']) # Return http response - if has_new_record_info: - checker = table_permissions_checker(new_record_info['collection'].id, - new_record_info['specify_user'], - "read") - return http.HttpResponse(api.toJson(api._obj_to_data(obj, checker)), - content_type='application/json', - status=204) - else: - return http.HttpResponse('', status=204) - -@openapi(schema={ - 'post': { - "requestBody": { - "required": True, - "description": "Replace a new model for an old model.", - "content": { - "application/x-www-form-urlencoded": { - "schema": { - "type": "object", - "description": "The error.", - "properties": { - "model_name": { - "type": "string", - "description": "The name of the table that is to be merged." - }, - "old_model_id": { - "type": "integer", - "description": "The old ID value of the model record that is to be replaced by the new one." - }, - "new_model_id": { - "type": "integer", - "description": "The new ID value of the model that is replacing the old one." - } - }, - 'required': ['model_name', 'old_model_id', 'new_model_id'], - 'additionalProperties': False - } - } - } - }, - "responses": { - "204": {"description": "Success",}, - "404": {"description": "The ID specified does not exist."}, - "405": {"description": "A database rule was broken."} - } - }, -}) -@login_maybe_required -@require_POST -def record_merge(request: http.HttpRequest, model_name: str, old_model_id: int, new_model_id: int) -> http.HttpResponse: - """Replaces all the foreign keys referencing the old record ID - with the new record ID, and deletes the old record. - """ - check_permission_targets(None, request.specify_user.id, [ReplaceRecordPT.update, ReplaceRecordPT.delete]) - - # TODO: Test reocord merging targeting records other than Agent before allowing generic merging - # if model_name.lower() != 'Agent'.lower(): - # return http.HttpResponseForbidden(f'Record Merging for the {model_name} table is not yet been fully tested.') - - response = record_merge_fx(model_name, [old_model_id], new_model_id) - return response - + return http.HttpResponse('', status=204) + @openapi(schema={ 'post': { "requestBody": { @@ -630,38 +570,30 @@ def record_merge(request: http.HttpRequest, model_name: str, old_model_id: int, } }, }) - @login_maybe_required @require_POST -def record_merge_extended(request: http.HttpRequest, model_name: str, new_model_id: int) -> http.HttpResponse: +def record_merge(request: http.HttpRequest, model_name: str, new_model_id: int) -> http.HttpResponse: """Replaces all the foreign keys referencing the old record IDs with the new record ID, and deletes the old records. """ request_params = http.QueryDict(request.META['QUERY_STRING']) - version = None - try: - version = request_params['version'] - except KeyError: - try: - version = request.META['HTTP_IF_MATCH'] - except KeyError: - version = None - if version is None: - version = getattr(spmodels, model_name.title()).objects.get(id=new_model_id).version + record_version = getattr(spmodels, model_name.title()).objects.get(id=new_model_id).version + get_version = request.GET.get('version', record_version) + version = get_version if isinstance(get_version, int) else 0 - checker = table_permissions_checker(request.specify_collection, request.specify_user_agent, "read") - check_permission_targets(None, request.specify_user.id, [ReplaceRecordPT.update, ReplaceRecordPT.delete]) + table_permissions_checker(request.specify_collection, request.specify_user_agent, "read") + check_permission_targets(request.specify_collection.id, request.specify_user.id, [ReplaceRecordPT.update, ReplaceRecordPT.delete]) data = json.loads(request.body) old_model_ids = data['old_record_ids'] new_record_info = { - 'agent_id': new_model_id, + 'agent_id': int(new_model_id), 'collection': request.specify_collection, 'specify_user': request.specify_user_agent, 'version': version, - 'new_record_data': data['new_record_data'] + 'new_record_data': data['new_record_data'] if 'new_record_data' in data else None } - response = record_merge_fx(model_name, old_model_ids, new_model_id, new_record_info) + response = record_merge_fx(model_name, old_model_ids, int(new_model_id), new_record_info) return response \ No newline at end of file From 03f11a11dcd02dd94c1c28629d514d15794a875a Mon Sep 17 00:00:00 2001 From: melton-jason Date: Tue, 16 May 2023 13:08:55 -0500 Subject: [PATCH 05/10] Modify frontend to use modified endpoint --- .../js_src/lib/components/Merging/index.tsx | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx index 09a34fb89de..523404ed5c1 100644 --- a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx @@ -227,26 +227,31 @@ function Merging({ * can leave the UI in a state consistent with the back-end */ // eslint-disable-next-line functional/no-loop-statement - for (const clone of clones) { - const response = await ajax( - `/api/specify/${model.name.toLowerCase()}/replace/${ - clone.id - }/${target.id}/`, - { - method: 'POST', - headers: { - Accept: 'text/plain', - }, - expectedErrors: [Http.NOT_ALLOWED], - errorMode: 'dismissible', - } - ); - if (response.status === Http.NOT_ALLOWED) { - setError(response.data); - return; + const response = await ajax( + `/api/specify/${model.name.toLowerCase()}/replace/${ + target.id + }/`, + { + method: 'POST', + headers: { + Accept: 'text/plain', + }, + body: { + old_record_ids: clones.map((clone) => clone.id), + new_record_data: merged.toJSON(), + }, + expectedErrors: [Http.NOT_ALLOWED], + errorMode: 'dismissible', } + ); + if (response.status === Http.NOT_ALLOWED) { + setError(response.data); + return; + } + for (const clone of clones) { resourceEvents.trigger('deleted', clone); } + setError(undefined); handleClose(); }) From 8d5f8f6b188a4b6c18f04b7c22a5610dccb5dabe Mon Sep 17 00:00:00 2001 From: melton-jason Date: Tue, 16 May 2023 14:44:47 -0500 Subject: [PATCH 06/10] Remove uneeded save to mergeResource --- .../js_src/lib/components/Merging/index.tsx | 51 ++++++++----------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx index 523404ed5c1..248b78a54cf 100644 --- a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx @@ -9,7 +9,6 @@ import { commonText } from '../../localization/common'; import { mergingText } from '../../localization/merging'; import { treeText } from '../../localization/tree'; import { ajax } from '../../utils/ajax'; -import { hijackBackboneAjax } from '../../utils/ajax/backboneAjax'; import { Http } from '../../utils/ajax/definitions'; import { f } from '../../utils/functools'; import type { RA } from '../../utils/types'; @@ -215,35 +214,27 @@ function Merging({ const clones = resources.slice(1); loading( - hijackBackboneAjax( - [], - async () => target.save(), - undefined, - 'dismissible' - ).then(async () => { - /* - * Make requests sequentially as they are expected to fail - * (due to business rules). If we do them sequentially, we - * can leave the UI in a state consistent with the back-end - */ - // eslint-disable-next-line functional/no-loop-statement - const response = await ajax( - `/api/specify/${model.name.toLowerCase()}/replace/${ - target.id - }/`, - { - method: 'POST', - headers: { - Accept: 'text/plain', - }, - body: { - old_record_ids: clones.map((clone) => clone.id), - new_record_data: merged.toJSON(), - }, - expectedErrors: [Http.NOT_ALLOWED], - errorMode: 'dismissible', - } - ); + /* + * Make requests sequentially as they are expected to fail + * (due to business rules). If we do them sequentially, we + * can leave the UI in a state consistent with the back-end + */ + // eslint-disable-next-line functional/no-loop-statement + ajax( + `/api/specify/${model.name.toLowerCase()}/replace/${target.id}/`, + { + method: 'POST', + headers: { + Accept: 'text/plain', + }, + body: { + old_record_ids: clones.map((clone) => clone.id), + new_record_data: merged.toJSON(), + }, + expectedErrors: [Http.NOT_ALLOWED], + errorMode: 'dismissible', + } + ).then((response) => { if (response.status === Http.NOT_ALLOWED) { setError(response.data); return; From ed4be7b9ca20982cec2d35f0cf9f59f06147a952 Mon Sep 17 00:00:00 2001 From: Max Patiiuk Date: Tue, 16 May 2023 17:49:11 -0500 Subject: [PATCH 07/10] Fix business rule error on merging agent with user --- specifyweb/frontend/js_src/lib/components/Forms/Save.tsx | 2 +- .../frontend/js_src/lib/components/Merging/index.tsx | 8 ++++++-- .../frontend/js_src/lib/utils/__tests__/utils.test.ts | 1 - specifyweb/frontend/js_src/lib/utils/utils.ts | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/Forms/Save.tsx b/specifyweb/frontend/js_src/lib/components/Forms/Save.tsx index 8f7a98098b6..4f6bb2a5d34 100644 --- a/specifyweb/frontend/js_src/lib/components/Forms/Save.tsx +++ b/specifyweb/frontend/js_src/lib/components/Forms/Save.tsx @@ -16,7 +16,6 @@ import { Submit } from '../Atoms/Submit'; import { LoadingContext } from '../Core/Contexts'; import type { AnySchema } from '../DataModel/helperTypes'; import type { SpecifyResource } from '../DataModel/legacyTypes'; -import { resourceOn } from '../DataModel/resource'; import type { Tables } from '../DataModel/types'; import { error } from '../Errors/assert'; import { Dialog } from '../Molecules/Dialog'; @@ -25,6 +24,7 @@ import { userPreferences } from '../Preferences/userPreferences'; import { smoothScroll } from '../QueryBuilder/helpers'; import { FormContext } from './BaseResourceView'; import { FORBID_ADDING, NO_CLONE } from './ResourceView'; +import { resourceOn } from '../DataModel/resource'; export const saveFormUnloadProtect = formsText.unsavedFormUnloadProtect(); diff --git a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx index 248b78a54cf..d2b465d62c0 100644 --- a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx @@ -13,7 +13,7 @@ import { Http } from '../../utils/ajax/definitions'; import { f } from '../../utils/functools'; import type { RA } from '../../utils/types'; import { filterArray } from '../../utils/types'; -import { removeKey, sortFunction } from '../../utils/utils'; +import { multiSortFunction, removeKey } from '../../utils/utils'; import { ErrorMessage } from '../Atoms'; import { Button } from '../Atoms/Button'; import { Input, Label } from '../Atoms/Form'; @@ -207,7 +207,11 @@ function Merging({ * and, presumably the longest auditing history */ const resources = Array.from(rawResources).sort( - sortFunction((resource) => resource.get('timestampCreated')) + multiSortFunction( + (resource) => resource.get('specifyUser'), + true, + (resource) => resource.get('timestampCreated') + ) ); const target = resources[0]; target.bulkSet(removeKey(merged.toJSON(), 'version')); diff --git a/specifyweb/frontend/js_src/lib/utils/__tests__/utils.test.ts b/specifyweb/frontend/js_src/lib/utils/__tests__/utils.test.ts index 10255a49e03..6337b3dec4e 100644 --- a/specifyweb/frontend/js_src/lib/utils/__tests__/utils.test.ts +++ b/specifyweb/frontend/js_src/lib/utils/__tests__/utils.test.ts @@ -143,7 +143,6 @@ test('multiSortFunction', () => { ].sort( multiSortFunction( ({ type }) => type, - false, ({ priority }) => priority, true ) diff --git a/specifyweb/frontend/js_src/lib/utils/utils.ts b/specifyweb/frontend/js_src/lib/utils/utils.ts index 43f1805bc5b..620ffd9c09f 100644 --- a/specifyweb/frontend/js_src/lib/utils/utils.ts +++ b/specifyweb/frontend/js_src/lib/utils/utils.ts @@ -144,7 +144,7 @@ export const sortFunction = export const multiSortFunction = ( ...payload: readonly ( - | boolean + | true | ((value: ORIGINAL_TYPE) => Date | boolean | number | string) )[] ): ((left: ORIGINAL_TYPE, right: ORIGINAL_TYPE) => -1 | 0 | 1) => From d744c913d33ae06be73aa1a0b59560acd6f878b3 Mon Sep 17 00:00:00 2001 From: alec_dev Date: Thu, 18 May 2023 13:49:26 -0500 Subject: [PATCH 08/10] datetime object in api tests --- specifyweb/specify/api_tests.py | 21 +++++++++++---------- specifyweb/specify/views.py | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/specifyweb/specify/api_tests.py b/specifyweb/specify/api_tests.py index 38ec3c42fde..7c9fcda5320 100644 --- a/specifyweb/specify/api_tests.py +++ b/specifyweb/specify/api_tests.py @@ -2,6 +2,7 @@ Tests for api.py """ +from datetime import datetime import json from unittest import skip @@ -622,7 +623,7 @@ def test_record_recursive_merge(self): insitution_1 = models.Institution.objects.get(name='Test Institution') reference_work_1 = models.Referencework.objects.create( id=875, - timestampcreated="2022-11-30 14:36:56.000", + timestampcreated=datetime.strptime("2022-11-30 14:36:56.000", '%Y-%m-%d %H:%M:%S.%f'), referenceworktype=2, institution=insitution_1 ) @@ -633,16 +634,16 @@ def test_record_recursive_merge(self): ordernumber=7, agent=agent_1, referencework=reference_work_1, - timestampcreated="2022-11-30 14:34:51.000", - timestampmodified="2022-11-30 14:33:30.000" + timestampcreated=datetime.strptime("2022-11-30 14:34:51.000", '%Y-%m-%d %H:%M:%S.%f'), + timestampmodified=datetime.strptime("2022-11-30 14:33:30.000", '%Y-%m-%d %H:%M:%S.%f') ) models.Author.objects.create( id=2554, ordernumber=2, agent=agent_2, referencework=reference_work_1, - timestampcreated="2022-11-30 14:33:30.000", - timestampmodified="2022-11-30 14:36:56.000" + timestampcreated=datetime.strptime("2022-11-30 14:33:30.000", '%Y-%m-%d %H:%M:%S.%f'), + timestampmodified=datetime.strptime("2022-11-30 14:36:56.000", '%Y-%m-%d %H:%M:%S.%f') ) # Assert that the api request ran successfully @@ -685,13 +686,13 @@ def test_agent_address_replacement(self): # Create mock addresses models.Address.objects.create( id=1, - timestampcreated="22022-11-30 14:34:51.000", + timestampcreated=datetime.strptime("2022-11-30 14:34:51.000", '%Y-%m-%d %H:%M:%S.%f'), address="1234 Main St.", agent=agent_1 ) models.Address.objects.create( id=2, - timestampcreated="2022-11-30 14:33:30.000", + timestampcreated=datetime.strptime("2022-11-30 14:33:30.000", '%Y-%m-%d %H:%M:%S.%f'), address="5678 Rainbow Rd.", agent=agent_2 ) @@ -738,19 +739,19 @@ def test_agent_address_multiple_replacement(self): # Create mock addresses models.Address.objects.create( id=1, - timestampcreated="22022-11-30 14:34:51.000", + timestampcreated=datetime.strptime("2022-11-30 14:34:51.000", '%Y-%m-%d %H:%M:%S.%f'), address="1234 Main St.", agent=agent_1 ) models.Address.objects.create( id=2, - timestampcreated="2022-11-30 14:33:30.000", + timestampcreated=datetime.strptime("2022-11-30 14:33:30.000", '%Y-%m-%d %H:%M:%S.%f'), address="5678 Rainbow Rd.", agent=agent_2 ) models.Address.objects.create( id=3, - timestampcreated="2022-11-30 14:32:30.000", + timestampcreated=datetime.strptime("2022-11-30 14:32:30.000", '%Y-%m-%d %H:%M:%S.%f'), address="2468 Mass St.", agent=agent_3 ) diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index fd2c26db6e1..d8b55abdbe3 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -530,7 +530,7 @@ def update_record(record: models.Model): 'post': { "requestBody": { "required": True, - "description": "Rplace a list of old records with a new record.", + "description": "Replace a list of old records with a new record.", "content": { "application/json": { "schema": { From cdef0a7cd22eac75f88b3f2f907b5228f2f2548f Mon Sep 17 00:00:00 2001 From: alec_dev Date: Thu, 18 May 2023 16:10:34 -0500 Subject: [PATCH 09/10] remove unused variable --- specifyweb/specify/views.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index d8b55abdbe3..fb4ae04c26f 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -576,8 +576,6 @@ def record_merge(request: http.HttpRequest, model_name: str, new_model_id: int) """Replaces all the foreign keys referencing the old record IDs with the new record ID, and deletes the old records. """ - request_params = http.QueryDict(request.META['QUERY_STRING']) - record_version = getattr(spmodels, model_name.title()).objects.get(id=new_model_id).version get_version = request.GET.get('version', record_version) version = get_version if isinstance(get_version, int) else 0 From 2b1069535da06e6b0cd4b36b3574f407f67758ff Mon Sep 17 00:00:00 2001 From: melton-jason Date: Fri, 19 May 2023 09:21:32 -0500 Subject: [PATCH 10/10] Make small refactoring changes --- specifyweb/frontend/js_src/lib/components/Merging/index.tsx | 5 ----- specifyweb/specify/views.py | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx index d2b465d62c0..c54036c2c39 100644 --- a/specifyweb/frontend/js_src/lib/components/Merging/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/Merging/index.tsx @@ -218,11 +218,6 @@ function Merging({ const clones = resources.slice(1); loading( - /* - * Make requests sequentially as they are expected to fail - * (due to business rules). If we do them sequentially, we - * can leave the UI in a state consistent with the back-end - */ // eslint-disable-next-line functional/no-loop-statement ajax( `/api/specify/${model.name.toLowerCase()}/replace/${target.id}/`, diff --git a/specifyweb/specify/views.py b/specifyweb/specify/views.py index fb4ae04c26f..e006597cd70 100644 --- a/specifyweb/specify/views.py +++ b/specifyweb/specify/views.py @@ -514,7 +514,7 @@ def update_record(record: models.Model): target_model.objects.get(id=old_model_id).delete() # Update new record with json info, if given - has_new_record_info = False if new_record_info is None else True + has_new_record_info = new_record_info is not None if has_new_record_info and 'new_record_data' in new_record_info and new_record_info['new_record_data'] is not None: obj = api.put_resource(new_record_info['collection'], new_record_info['specify_user'],