Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
04be658
[kbss-cvut/record-manager-ui#302] Add variable for allowing creation …
palagdan Oct 29, 2025
8f4d0bd
[kbss-cvut/record-manager-ui#302] Showing also records without instit…
palagdan Oct 29, 2025
0158ab5
[kbss-cvut/record-manager-ui#302] Move permission's validation to the…
palagdan Oct 30, 2025
a779307
[kbss-cvut/record-manager-ui#302] Fix and implement new tests
palagdan Oct 30, 2025
f4f2224
[kbss-cvut/record-manager-ui#302] Change records institution after ch…
palagdan Oct 31, 2025
d39b1cf
[kbss-cvut/record-manager-ui#302] Fix modifiers
palagdan Oct 31, 2025
b31e3c7
[kbss-cvut/record-manager-ui#302] Improve validation error
blcham Oct 31, 2025
fd92863
[kbss-cvut/record-manager-ui#302] Indent where clauses
palagdan Nov 1, 2025
ab853a0
[kbss-cvut/record-manager-ui#302] Change environment variable descrip…
palagdan Nov 1, 2025
89cced0
[kbss-cvut/record-manager-ui#302] Fix test name
palagdan Nov 1, 2025
acd2dc5
[kbss-cvut/record-manager-ui#302] Fix Record naming after rebasing
palagdan Nov 2, 2025
c02d903
[kbss-cvut/record-manager-ui#302] Fix record persist/update without i…
blcham Mar 17, 2026
a287cad
[kbss-cvut/record-manager-ui#302] Fix pagination results
blcham Mar 17, 2026
a318a13
[kbss-cvut/record-manager-ui#302] Fix institution comparison in preUp…
blcham Mar 17, 2026
5fff414
[kbss-cvut/record-manager-ui#302] Rename changePatientRecordsInstitut…
blcham Mar 17, 2026
38b926a
[kbss-cvut/record-manager-ui#302] Allow user to view own record witho…
blcham Mar 18, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/cz/cvut/kbss/study/model/Institution.java
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,19 @@ public void setReferenceId(Integer referenceId) {
this.referenceId = referenceId;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof Institution institution)) return false;
if (uri == null) return false;
return uri.equals(institution.uri);
}

@Override
public int hashCode() {
return uri != null ? uri.hashCode() : System.identityHashCode(this);
}

@Override
public String toString() {
return "Institution{<" + uri +
Expand Down
73 changes: 48 additions & 25 deletions src/main/java/cz/cvut/kbss/study/persistence/dao/RecordDao.java
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,16 @@ public int getNumberOfProcessedRecords() {
* @param entity The local name to be checked for uniqueness
*/
public void requireUniqueNonEmptyLocalName(Record entity) {
Objects.requireNonNull(entity.getInstitution());
if (entity.getLocalName() == null || entity.getLocalName().isEmpty()) {
throw new ValidationException("error.record.localNameOfRecordIsEmpty",
"Local name of record is empty for entity " + entity);
}

if(entity.getInstitution() == null){
em.clear();
return;
}

boolean unique = findByInstitution(entity.getInstitution()).stream()
.filter(pr -> (entity.getFormTemplate() != null) && entity.getFormTemplate()
.equals(pr.getFormTemplate()))
Expand Down Expand Up @@ -305,37 +310,55 @@ private void setQueryParameters(Query query, Map<String, Object> queryParams) {

private static String constructWhereClause(RecordFilterParams filters, Map<String, Object> queryParams) {
// Could not use Criteria API because it does not support OPTIONAL
String whereClause = "{" +
"?r a ?type ; " +
"?hasAuthor ?author ; " +
"?hasCreatedDate ?created ; " +
"?hasInstitution ?institution . " +
"?institution ?hasKey ?institutionKey ." +
"?author ?hasUsername ?username ." +
"OPTIONAL { ?r ?hasPhase ?phase . } " +
"OPTIONAL { ?r ?hasFormTemplate ?formTemplate . } " +
"OPTIONAL { ?r ?hasLastModified ?lastModified . } " +
"BIND (COALESCE(?lastModified, ?created) AS ?date) ";

String whereClause = """
{
?r a ?type ;
?hasAuthor ?author ;
?hasCreatedDate ?created .
?author ?hasUsername ?username .

OPTIONAL {
?r ?hasInstitution ?institution .
?institution ?hasKey ?institutionKey .
}

OPTIONAL { ?r ?hasPhase ?phase . }
OPTIONAL { ?r ?hasFormTemplate ?formTemplate . }
OPTIONAL { ?r ?hasLastModified ?lastModified . }

BIND (COALESCE(?lastModified, ?created) AS ?date)
""";

whereClause += mapParamsToQuery(filters, queryParams);
whereClause += "}";

return whereClause;
}

private static String constructWhereClauseWithGraphs(RecordFilterParams filters, Map<String, Object> queryParams) {
// Could not use Criteria API because it does not support OPTIONAL
String whereClause = "{GRAPH ?r{" +
"?r a ?type ; " +
"?hasCreatedDate ?created ; " +
"?hasInstitution ?institution . " +
"OPTIONAL { ?r ?hasPhase ?phase . } " +
"OPTIONAL { ?r ?hasFormTemplate ?formTemplate . } " +
"OPTIONAL { ?r ?hasLastModified ?lastModified . } " +
"BIND (COALESCE(?lastModified, ?created) AS ?date) ";
whereClause += mapParamsToQuery(filters, queryParams);
whereClause += "}" +
"GRAPH ?institutionGraph{" +
"?institution ?hasKey ?institutionKey ." +
"}}";
String whereClause = """
{GRAPH ?r {
?r a ?type ;
?hasCreatedDate ?created .

OPTIONAL { ?r ?hasInstitution ?institution . }
OPTIONAL { ?r ?hasPhase ?phase . }
OPTIONAL { ?r ?hasFormTemplate ?formTemplate . }
OPTIONAL { ?r ?hasLastModified ?lastModified . }

BIND (COALESCE(?lastModified, ?created) AS ?date)
"""
+ mapParamsToQuery(filters, queryParams) +
"""
}
OPTIONAL {
GRAPH ?institutionGraph {
?institution ?hasKey ?institutionKey .
}
}}
""";

queryParams.put("institutionGraph", URI.create(Vocabulary.s_c_institution + "s"));

Expand Down
5 changes: 3 additions & 2 deletions src/main/java/cz/cvut/kbss/study/rest/RecordController.java
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ public ResponseEntity<InputStreamResource> exportRecordsExcel(MultiValueMap<Stri
.body(new InputStreamResource(stream));
}

@PreAuthorize("hasAuthority('" + SecurityConstants.readAllRecords + "') " +
@PreAuthorize("@securityUtils.isRecordCreatedByUser(#key) " +
"or hasAuthority('" + SecurityConstants.readAllRecords + "') " +
"or (hasAuthority('" + SecurityConstants.readOrganizationRecords + "') and @securityUtils.isRecordInUsersInstitution(#key))")
@GetMapping(value = "/{key}", produces = MediaType.APPLICATION_JSON_VALUE)
public Record getRecord(@PathVariable("key") String key) {
Expand All @@ -168,7 +169,7 @@ private Record findInternal(String key) {
@ResponseStatus(HttpStatus.CREATED)
public ResponseEntity<String> createRecord(@RequestBody Record record) {

if(userService.getCurrentUser().getInstitution() == null)
if(configReader.getConfig(ConfigParam.RECORDS_ALLOWED_CREATION_WITHOUT_INSTITUTION).equals("false") && userService.getCurrentUser().getInstitution() == null)
throw new ValidationException("record.save-error.user-not-assigned-to-institution",
"User is not assigned to any institution.");

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package cz.cvut.kbss.study.service.repository;

import cz.cvut.kbss.jopa.exceptions.EntityNotFoundException;
import cz.cvut.kbss.study.dto.RecordDto;
import cz.cvut.kbss.study.exception.EntityExistsException;
import cz.cvut.kbss.study.exception.NotFoundException;
import cz.cvut.kbss.study.exception.ValidationException;
import cz.cvut.kbss.study.model.Institution;
import cz.cvut.kbss.study.model.Record;
import cz.cvut.kbss.study.model.Role;
import cz.cvut.kbss.study.model.User;
import cz.cvut.kbss.study.persistence.dao.GenericDao;
import cz.cvut.kbss.study.persistence.dao.RecordDao;
import cz.cvut.kbss.study.persistence.dao.UserDao;
import cz.cvut.kbss.study.persistence.dao.util.RecordFilterParams;
import cz.cvut.kbss.study.service.ConfigReader;
import cz.cvut.kbss.study.service.EmailService;
import cz.cvut.kbss.study.service.UserService;
Expand All @@ -26,11 +29,8 @@
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.*;
import java.util.stream.Collectors;

@Service
public class RepositoryUserService extends BaseRepositoryService<User> implements UserService {
Expand Down Expand Up @@ -207,23 +207,15 @@ protected void prePersist(User instance) {
}
}

@Override
protected void preUpdate(User instance) {
final User currentUser = securityUtils.getCurrentUser();
final User original = userDao.findByUsername(instance.getUsername());

if(original == null) {
throw new EntityNotFoundException("User with specified username does not exist.");
}

boolean differentUser = !Objects.equals(instance.getUsername(), currentUser.getUsername());
protected void validatePermissionToUpdate(User currentUser, User toUpdate, User original) {
boolean differentUser = !Objects.equals(toUpdate.getUsername(), currentUser.getUsername());

boolean hasWriteAllUsers = securityUtils.hasRole(Role.writeAllUsers);
boolean lacksPrivilegeOfUpdatedUser = !securityUtils.hasSupersetOfRoles(currentUser, original);

boolean sameInstitution = instance.getInstitution() != null
boolean sameInstitution = toUpdate.getInstitution() != null
&& currentUser.getInstitution() != null
&& instance.getInstitution().getKey().equals(currentUser.getInstitution().getKey());
&& toUpdate.getInstitution().getKey().equals(currentUser.getInstitution().getKey());

boolean hasWriteOrganizationUsers = securityUtils.hasRole(Role.writeOrganizationUsers) && sameInstitution;

Expand All @@ -243,6 +235,71 @@ protected void preUpdate(User instance) {
currentUser.getUsername()));
}
}
}

protected void validateRecordsAgainstCollisions(User toUpdate, User original) {

@blcham blcham Oct 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@palagdan, look at my last commit. I had the following issues:

    1. to me, variable names were not correct, e.g,. newInsitutionRecords ==> existingInstitutionRecords
    1. 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
    1. 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 :)
    1. IMPORTANT: patient records --> records (we have technical debt there :( )

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think there might be problem within UI how to show this message due to \n in the message and how it is interpreted in HTML (did not test it)

@blcham blcham Oct 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't like the solution, revert it and address please at least IMPORTANT bullets.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution :)


if (toUpdate.getInstitution() == null) return;

Map<String, RecordDto> existingInstitutionRecords = recordDao.findByInstitution(toUpdate.getInstitution()).stream().collect(
Collectors.toMap(RecordDto::getLocalName, r -> r)
);
Map<String, Record> newInstitutionRecords = recordDao.findByAuthor(original).stream().collect(
Collectors.toMap(Record::getLocalName, r -> r)
);

Map<String, List<String>> conflictingRecords = newInstitutionRecords.keySet().stream()
.filter(existingInstitutionRecords::containsKey)
.collect(Collectors.toMap(
key -> key,
key -> Arrays.asList(existingInstitutionRecords.get(key).getKey(), newInstitutionRecords.get(key).getKey())
));


if (!conflictingRecords.isEmpty()) {

String conflictsFormatted = conflictingRecords.entrySet().stream()
.map(e -> String.format(
"• %s (existing record ID: %s, new record ID: %s)",
e.getKey(),
e.getValue().get(0),
e.getValue().get(1)
))
.collect(Collectors.joining("\n"));

String message = String.format(
"User cannot be moved to institution '%s' because there are conflicting records with the same name:\n%s.",
toUpdate.getInstitution().getName(),
conflictsFormatted
);

throw new ValidationException(message);
}
}

protected void changeRecordsInstitution(User original, Institution newInstitution) {
List<Record> recordsToUpdate = recordDao.findByAuthor(original);
for (Record record : recordsToUpdate) {
record.setInstitution(newInstitution);
recordDao.update(record);
}
}

@Override
protected void preUpdate(User instance) {
final User currentUser = securityUtils.getCurrentUser();
final User original = userDao.findByUsername(instance.getUsername());

if (original == null) {
throw new EntityNotFoundException("User with specified username does not exist.");
}

validatePermissionToUpdate(currentUser, instance, original);

if (!Objects.equals(instance.getInstitution(), original.getInstitution())) {
validateRecordsAgainstCollisions(instance, original);
changeRecordsInstitution(original, instance.getInstitution());
}

try {
Validator.validateEmail(instance.getEmailAddress());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,16 +155,33 @@ public boolean isMemberOfInstitution(String institutionKey) {

/**
* Checks whether the current user is in same institution as the record was created.
* Returns false if user is not member of any institution.
*
* @param recordKey Record identifier
* @return Membership status of the current user and record
*/
public boolean isRecordInUsersInstitution(String recordKey) {
final User user = getCurrentUser();
final Record record = recordDao.findByKey(recordKey);
if (user.getInstitution() == null) {
return false;
}
return user.getInstitution().getKey().equals(record.getInstitution().getKey());
}

/**
* Checks whether the current user is the author of the record.
*
* @param recordKey Record identifier
* @return Membership status of the current user and record
*/
public boolean isRecordCreatedByUser(String recordKey) {
final User user = getCurrentUser();
final Record record = recordDao.findByKey(recordKey);

return record.getAuthor() != null && record.getAuthor().equals(user);
}

/**
* Checks whether the current user is in same institution as user we are asking for.
*
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/cz/cvut/kbss/study/util/ConfigParam.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ public enum ConfigParam {

CORS_ALLOWED_ORIGINS("security.cors.allowedOrigins"),

RECORDS_ALLOWED_REJECT_REASON("records.allowedRejectReason");
RECORDS_ALLOWED_REJECT_REASON("records.allowedRejectReason"),
RECORDS_ALLOWED_CREATION_WITHOUT_INSTITUTION("records.allowedCreationWithoutInstitution");

private final String name;

Expand Down
13 changes: 13 additions & 0 deletions src/main/java/cz/cvut/kbss/study/util/Configuration.java
Original file line number Diff line number Diff line change
Expand Up @@ -422,12 +422,25 @@ public static class Records {
*/
boolean allowedRejectReason;

/**
* Allow users who are not members of any institution to create records.
*/
boolean allowedCreationWithoutInstitution;

public boolean isAllowedRejectReason() {
return allowedRejectReason;
}

public boolean isAllowedCreationWithoutInstitution() {
return allowedCreationWithoutInstitution;
}

public void setAllowedRejectReason(boolean allowedRejectReason) {
this.allowedRejectReason = allowedRejectReason;
}

public void setAllowedCreationWithoutInstitution(boolean allowedCreationWithoutInstitution) {
this.allowedCreationWithoutInstitution = allowedCreationWithoutInstitution;
}
}
}
1 change: 1 addition & 0 deletions src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,4 @@ email:

records:
allowedRejectReason: true
allowedCreationWithoutInstitution: true
23 changes: 22 additions & 1 deletion src/test/java/cz/cvut/kbss/study/rest/RecordControllerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ public void createRecordReturnsResponseStatusCreated() throws Exception {
Record record = Generator.generateRecord(user);
when(userService.getCurrentUser()).thenReturn(user);

when(configReaderMock.getConfig(ConfigParam.RECORDS_ALLOWED_CREATION_WITHOUT_INSTITUTION))
.thenReturn("false");

final MvcResult result = mockMvc.perform(post("/records").content(toJson(record))
.contentType(MediaType.APPLICATION_JSON_VALUE))
.andReturn();
Expand All @@ -211,12 +214,14 @@ public void createRecordReturnsResponseStatusCreated() throws Exception {
}

@Test
public void createRecordWithoutInstitutionReturnsResponseStatusBadRequest() throws Exception {
public void createRecordWithoutInstitutionIfItIsNotAllowedReturnsResponseStatusBadRequest() throws Exception {
user.setInstitution(null);

Record record = Generator.generateRecord(user);

when(userService.getCurrentUser()).thenReturn(user);
when(configReaderMock.getConfig(ConfigParam.RECORDS_ALLOWED_CREATION_WITHOUT_INSTITUTION))
.thenReturn("false");

final MvcResult result = mockMvc.perform(post("/records").content(toJson(record))
.contentType(MediaType.APPLICATION_JSON_VALUE))
Expand All @@ -225,6 +230,22 @@ public void createRecordWithoutInstitutionReturnsResponseStatusBadRequest() thro
assertEquals(HttpStatus.CONFLICT, HttpStatus.valueOf(result.getResponse().getStatus()));
}

@Test
public void createRecordWithoutInstitutionIfItIsAllowedReturnsResponseStatusCreated() throws Exception {
user.setInstitution(null);

Record record = Generator.generateRecord(user);

when(configReaderMock.getConfig(ConfigParam.RECORDS_ALLOWED_CREATION_WITHOUT_INSTITUTION))
.thenReturn("true");

final MvcResult result = mockMvc.perform(post("/records").content(toJson(record))
.contentType(MediaType.APPLICATION_JSON_VALUE))
.andReturn();

assertEquals(HttpStatus.CREATED, HttpStatus.valueOf(result.getResponse().getStatus()));
}


@Test
public void updateRecordReturnsResponseStatusNoContent() throws Exception {
Expand Down
Loading