-
Notifications
You must be signed in to change notification settings - Fork 9
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
feature: Resources page with routing and list of resource types #788
Conversation
loadingPermissions = computed(() => { | ||
if (this.authService.restrictions().length > 0) { | ||
this.getUserPermissions(); | ||
} else { | ||
return `<div>Could not load permissions</div>`; | ||
} | ||
}); |
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.
see commit a9f6e80 as an example on how to improve this
List<ResourceTypeEntity> predefinedResourceTypes = new ArrayList<>(); | ||
for (ResourceTypeEntity e : resourceTypeDomainService.getResourceTypes()) { | ||
if (e.getParentResourceType() == null && ResourceType.createByResourceType(e, null).isDefaultResourceType()) { | ||
predefinedResourceTypes.add(e); | ||
} | ||
} | ||
if (predefinedResourceTypes.isEmpty()) { | ||
throw new NotFoundException("No predefined resource types found"); | ||
} | ||
return predefinedResourceTypes; | ||
} |
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.
I assume that this is new code (or taken from one of the view beans?)
Try to use modern java - with lambdas and streams (maybe optionals) - basically, there is almost never a reason to use for loops.
so this could look like this:
public List<ResourceTypeEntity> getPredefinedResourceTypes() throws NotFoundException {
List<ResourceTypeEntity> predefinedResourceTypes = resourceTypeDomainService.getResourceTypes()
.stream()
.filter(resourceTypeEntity -> resourceTypeEntity == null && ResourceType.createByResourceType(resourceTypeEntity, null).isDefaultResourceType())
.collect(Collectors.toList());
if (predefinedResourceTypes.isEmpty()) {
throw new NotFoundException("No predefined resource types found");
}
return predefinedResourceTypes;
}
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.
I also think that the NotFoundException should not be thrown here. The exception is only relevant for the rest point (where the NotFoundExcpetionMapper exists).
It might be perfectly valid to get an empty list for other use cases in the future. Additionally - it will throw an error in the frontend. The more I think about it - just remove the exception - an empty list should not return an HTTP status 404.
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.
GET /thing/:id
should return 404 if the thing with the given id was not found
GET /thing
should not return 404 if the list is empty
@ApiOperation(value = "Get all available ResourceTypes - used by Angular") | ||
public List<ResourceTypeDTO> getAllResourceTypes() throws ValidationException { | ||
@ApiOperation(value = "Get all resource types") | ||
public List<ResourceTypeDTO> getAllResourceTypes() throws NotFoundException { |
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.
some as above - empty lists should not return a 404
for (ResourceTypeEntity resourceType : resourceTypes) { | ||
resourceTypeDTOs.add(new ResourceTypeDTO(resourceType)); | ||
} | ||
return resourceTypeDTOs; |
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.
replace with stream().map()
List<ResourceTypeEntity> resourceTypes = resourceTypeLocator.getPredefinedResourceTypes(); | ||
List<ResourceTypeDTO> resourceTypeDTOs = new ArrayList<>(); | ||
for (ResourceTypeEntity resourceType : resourceTypes) { | ||
resourceTypeDTOs.add(new ResourceTypeDTO(resourceType)); |
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.
replace with stream().map()
List<ResourceTypeEntity> resourceTypes = resourceTypeLocator.getRootResourceTypes(); | ||
List<ResourceTypeDTO> resourceTypeDTOs = new ArrayList<>(); | ||
for (ResourceTypeEntity resourceType : resourceTypes) { | ||
resourceTypeDTOs.add(new ResourceTypeDTO(resourceType)); | ||
} | ||
return resourceTypeDTOs; |
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.
replace with stream().map()
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.
It would also have been nice to split this PR along the stories:
- create page skeleton and routing in the frontend
- show List of ResourceTypes
305be7d
to
842ce79
Compare
e5630cb
to
27d431e
Compare
<span>{{ expandedResourceTypeId === resourceType.id ? '-' : '+' }}</span> | ||
} | ||
</a> | ||
@if (expandedResourceTypeId === resourceType.id) { |
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.
@if (expandedResourceTypeId === resourceType.id) { | |
@if (resourceType.hasChildren && expandedResourceTypeId === resourceType.id) { |
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.
Suggestion above:
Because otherwise you can also expand those that have no children, only then an "empty list" or simply no list is displayed, but you can see that something has been expanded in the UI.
02dd6e3
to
783f021
Compare
No description provided.