-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove merging capability from forms #3842
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of simply removing something because of an issue.
Finding a solution would be infinitely better.
However, this is not a major component of record merging, and there is considerable pressure to get the release out, which is why this is a barely acceptable change.
If we do remove a feature from the code, it should be easy to identify what the previous implementation was, and/or why it was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like Jason said, I don't like this solution, but for not delaying the release let's have it.
For a proper fix, you can just tell the Search Dialog to exclude the current record using an extra Filter:
readonly extraFilters: RA<QueryComboBoxFilter<SCHEMA>> | undefined; |
edit: looks like that is already done
specify7/specifyweb/frontend/js_src/lib/components/FormMeta/MergeRecord.tsx
Lines 61 to 66 in 207df9a
{ | |
field: 'id', | |
operation: 'in', | |
isNot: true, | |
value: resource.id.toString(), | |
}, |
the reason it doesn't work is that those extra filters were only respected by regular search, not query builder search until xml-editor branch (this branch likely doesn't have the fix)
thus the issue would likely fix itself in 7.10
if not, just call deduplicate selected resources once selected and abort if there is only one
The actual fix would be on the backend - it is integrity error if trying to merge into self. That is, the backend should bail out in that case. It shouldn't be just the frontend catch for this. |
Fixes #3836