feat: default access form selection implementation#1127
Conversation
ChrisiSailer
requested review from
RadovanTomik,
konradlang and
tmilost
as code owners
There was a problem hiding this comment.
Pull request overview
Implements backend-driven defaulting of accessForm during resource registration (using the organization’s most common access form, with a system fallback), and surfaces the chosen access form in the organization resource list UI.
Changes:
- Make “Access Form” optional in the resource creation modal (frontend).
- Default
accessFormon resource creation when omitted, backed by a new repository query (backend). - Expose
accessFormon resource representations used in organization/network resource listings and add integration tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/governance/CreateResourceModal.vue | Removes “required” semantics for Access Form so submission can rely on backend defaulting. |
| frontend/src/components/OrganizationListItem.vue | Displays the access form name (or “None assigned”) for each resource. |
| backend/src/test/java/eu/bbmri_eric/negotiator/governance/resource/ResourceControllerTests.java | Adds integration tests covering default access form behavior. |
| backend/src/main/java/eu/bbmri_eric/negotiator/governance/resource/dto/ResourceWithRepsDTO.java | Adds accessForm field to resource DTO used in expanded org/network views. |
| backend/src/main/java/eu/bbmri_eric/negotiator/governance/resource/dto/ResourceCreateDTO.java | Makes accessFormId optional by removing @NotNull. |
| backend/src/main/java/eu/bbmri_eric/negotiator/governance/resource/ResourceServiceImpl.java | Applies defaulting logic when accessFormId is missing/zero. |
| backend/src/main/java/eu/bbmri_eric/negotiator/form/repository/AccessFormRepository.java | Adds native query to find the most common access form per organization. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1127 +/- ##
============================================
+ Coverage 86.04% 86.06% +0.01%
- Complexity 2312 2316 +4
============================================
Files 291 291
Lines 6442 6450 +8
Branches 402 402
============================================
+ Hits 5543 5551 +8
Misses 645 645
Partials 254 254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .orElseThrow( | ||
| () -> new AccessFormNotFoundException(DEFAULT_ACCESS_FORM_ID))); | ||
| } else { | ||
| accessForm = |
There was a problem hiding this comment.
If we could try to reduce the queries to the database that would be great as well. This logic could be in a loop of lots of resources with a relatively small amount of access forms and organizations, but they are queried all the time. I see that this behavior was preexisting and we could address it in a separate issue as well.
Code for improving the performance here could look something like:
...
Map<Long, AccessForm> accessFormById = new HashMap<>();
Map<Long, AccessForm> fallbackAccessFormByOrganization = new HashMap<>();
Map<Long, DiscoveryService> discoveryServiceById = new HashMap<>();
for (ResourceCreateDTO resDTO : resourcesCreateDTO) {
DiscoveryService discoveryService = discoveryServiceById.computeIfAbsent(resDTO.getDiscoveryServiceId(),
id ->
discoveryServiceRepository
.findById(id)
.orElseThrow(
() -> new DiscoveryServiceNotFoundException(id)));
AccessForm accessForm;
if (resDTO.getAccessFormId() == null) {
accessForm = fallbackAccessFormByOrganization.computeIfAbsent(
resDTO.getOrganizationId(),
orgId ->
accessFormRepository
.findFirstMostCommonAccessFormByOrganization(orgId)
.orElseGet(() -> getAccessFormById(accessFormById, DEFAULT_ACCESS_FORM_ID));
} else {
accessForm = getAccessFormById(accessFormById, resDTO.getAccessFormId());
}
...
|
|
stetsche
left a comment
stetsche
left a comment
There was a problem hiding this comment.
Looking good now @ChrisiSailer ! Just some minor comments.
| + "ORDER BY COUNT(r.id) DESC " | ||
| + "LIMIT 1", | ||
| nativeQuery = true) | ||
| Optional<AccessForm> findFirstMostCommonAccessFormByOrganization(Long organizationId); |
There was a problem hiding this comment.
Some unit tests for the repository to test more cases then covered in the integration test would be great, but not a blocker.
|
|
Description:
This change allows users to use a default access form during resource registration, based on the most commonly used form within the selected organization.
Checklist:
Make sure you tick all the boxes below if they are true or do not apply before you ask for review
Required for all pull requests:
If applicable to this PR: