Support creation of records while not being assigned to any institution#101
Open
palagdan wants to merge 16 commits into
Open
Support creation of records while not being assigned to any institution#101palagdan wants to merge 16 commits into
palagdan wants to merge 16 commits into
Conversation
palagdan
force-pushed
the
302-create-records-without-institution
branch
from
October 31, 2025 14:52
6bd53b6 to
67f554c
Compare
blcham
requested changes
Oct 31, 2025
blcham
left a comment
blcham
left a comment
There was a problem hiding this comment.
I refactored your code; see the last commit. Have a look and if you don't like it revert it, but i would like to address issues described there.
| } | ||
| } | ||
|
|
||
| protected void validateRecordsAgainstCollisions(User toUpdate, User original) { |
There was a problem hiding this comment.
@palagdan, look at my last commit. I had the following issues:
-
- to me, variable names were not correct, e.g,. newInsitutionRecords ==> existingInstitutionRecords
-
- although not that important -- algorithm was of higher complexity than needed because of comparing every label with every other label, i.e., on average O(N^2) instead of O(N) on average
-
- IMPORTANT: I wanted to have a more detailed message on errors (although we don't have parametrized so we cannot navigate from the message to actual conflicting records but I would like to get there someday :)
-
- IMPORTANT: patient records --> records (we have technical debt there :( )
There was a problem hiding this comment.
- I think there might be problem within UI how to show this message due to
\nin the message and how it is interpreted in HTML (did not test it)
There was a problem hiding this comment.
if you don't like the solution, revert it and address please at least IMPORTANT bullets.
Collaborator
Author
There was a problem hiding this comment.
I like this solution :)
palagdan
force-pushed
the
302-create-records-without-institution
branch
from
November 1, 2025 10:34
909a706 to
d3a0c55
Compare
palagdan
force-pushed
the
302-create-records-without-institution
branch
2 times, most recently
from
November 2, 2025 12:13
967efab to
4081254
Compare
…records without being assigned to any institution
… separate function and implement record collision validation
…anging user institution
…nstitution Records without an institution will throw NPE during save/update instead of being allowed through.
Records without an institution were excluded from paginated results because ?hasInstitution ?institution was a required triple. Made it OPTIONAL and wrapped the GRAPH ?institutionGraph lookup in OPTIONAL as well.
…date to use URI equality Reference equality (!=) always evaluated to true when comparing deserialized request body institution against a DB-fetched institution, causing records to be unnecessarily migrated on every user update. Compare by URI instead.
blcham
force-pushed
the
302-create-records-without-institution
branch
from
March 17, 2026 14:47
f7dbf08 to
a318a13
Compare
…ion to changeRecordsInstitution
blcham
force-pushed
the
302-create-records-without-institution
branch
from
March 19, 2026 04:24
183a59d to
8ca98f9
Compare
…ut having read permissions
blcham
force-pushed
the
302-create-records-without-institution
branch
from
March 19, 2026 04:36
8ca98f9 to
38b926a
Compare
blcham
requested changes
Mar 19, 2026
blcham
left a comment
blcham
left a comment
There was a problem hiding this comment.
Reviewed and it looks ok, but testing it locally showed multiple issues that needs to be solved first, follow the test scenario:
-
- substitute RM with current branch
-
- add user without institution
-
- add a record (should be success) (related issue -- #106)
-
- set
records.allowedCreationWithoutInstitution: false
- set
-
- add a record (should be fail)
-
- set user an organization (his records should migrate as well)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolve partially kbss-cvut/record-manager-ui#302.