community
/
device-mgt-core
12
0
Fork 47
Code Issues Pull Requests 27 Packages Projects Releases Wiki Activity
Labels Milestones

device organization api implementation to get children of root nodes #353

Merged
tcdlpds merged 4 commits from isuri/device-mgt-core:deviceOrgImprovements into master 9 months ago
Conversation 0 Commits 4 Files Changed 35
isuri commented 9 months ago
There is no content yet.
isuri added 2 commits 9 months ago
isuri changed title from WIP: device organization api implementation to get children of root nodes to device organization api implementation to get children of root nodes 9 months ago
isuri requested review from tcdlpds 9 months ago
tcdlpds requested changes 9 months ago
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/dto/RootChildrenRequest.java Outdated
package io.entgra.device.mgt.core.device.mgt.extensions.device.organization.dto;
tcdlpds commented 9 months ago
Owner

Add the license

Add the license
isuri marked this conversation as resolved
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/impl/DeviceOrganizationServiceImpl.java Outdated
import io.entgra.device.mgt.core.device.mgt.extensions.device.organization.dto.DeviceNodeResult;
import io.entgra.device.mgt.core.device.mgt.extensions.device.organization.dto.DeviceOrganization;
import io.entgra.device.mgt.core.device.mgt.extensions.device.organization.dto.PaginationRequest;
import io.entgra.device.mgt.core.device.mgt.extensions.device.organization.dto.*;
tcdlpds commented 9 months ago
Owner

Do not use wild card imports

Do not use wild card imports
isuri marked this conversation as resolved
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/impl/DeviceOrganizationServiceImpl.java Outdated
allDeviceOrganizations.add(childrenResult);
}
return allDeviceOrganizations;
} catch (Exception e) {
tcdlpds commented 9 months ago
Owner

What is the reason for catching Exception here? Can't we catch the specific exceptions?

What is the reason for catching Exception here? Can't we catch the specific exceptions?
isuri marked this conversation as resolved
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/impl/DeviceOrganizationServiceImpl.java Outdated
return allDeviceOrganizations;
} catch (Exception e) {
String msg = "Error occurred while retrieving all device organizations for roots.";
log.error(msg);
tcdlpds commented 9 months ago
Owner

Log error as well.

Log error as well.
isuri marked this conversation as resolved
isuri added 1 commit 9 months ago
isuri requested review from tcdlpds 9 months ago
rajitha reviewed 9 months ago
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization.api/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/api/DeviceOrganizationMgtService.java Outdated
* @return A response containing a list of root device organizations.
*/
@GET
@Path("roots-children")
rajitha commented 9 months ago
Collaborator

Better to change the context path into /roots/children

Better to change the context path into /roots/children
isuri marked this conversation as resolved
rajitha reviewed 9 months ago
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/impl/DeviceOrganizationServiceImpl.java Outdated
PaginationRequest paginationRequest = new PaginationRequest(request.getOffSet(), request.getLimit());
List<DeviceOrganization> roots = getDeviceOrganizationRoots(paginationRequest);
if (roots == null || roots.isEmpty()) {
rajitha commented 9 months ago
Collaborator

What is the purpose of logging warn message for the empty list here? If there is no reason it is better to remove the warn log and can also omit the empty checking as well.

What is the purpose of logging warn message for the empty list here? If there is no reason it is better to remove the warn log and can also omit the empty checking as well.
isuri commented 9 months ago
Poster

If roots are empty or null when iterating over it there can be NullPointerExceptions. That is why the check is added. warn msg can be removed.

If roots are empty or null when iterating over it there can be NullPointerExceptions. That is why the check is added. warn msg can be removed.
rajitha commented 9 months ago
Collaborator

Iterating over an empty list isn't throw a null ptr. So

if (roots != null) {
	// run the for loop here
}

return allDeviceOrgs

Got the point?

Iterating over an empty list isn't throw a null ptr. So ``` if (roots != null) { // run the for loop here } return allDeviceOrgs ``` Got the point?
isuri commented 9 months ago
Poster

Yeah, it was a technical misunderstanding on my side.

Yeah, it was a technical misunderstanding on my side.
isuri marked this conversation as resolved
rajitha reviewed 9 months ago
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/impl/DeviceOrganizationServiceImpl.java Outdated
if (childrenResult != null) {
allDeviceOrganizations.add(childrenResult);
} else {
log.warn("no children found for roots.");
rajitha commented 9 months ago
Collaborator

If there is no reason, better to remove the warn log from the loop.

If there is no reason, better to remove the warn log from the loop.
isuri marked this conversation as resolved
rajitha reviewed 9 months ago
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/impl/DeviceOrganizationServiceImpl.java Outdated
}
}
return allDeviceOrganizations;
} catch (NullPointerException | DeviceOrganizationMgtPluginException e) {
rajitha commented 9 months ago
Collaborator

No need to catch the NullPointerException here since it's a runtime exception.

No need to catch the NullPointerException here since it's a runtime exception.
isuri marked this conversation as resolved
rajitha reviewed 9 months ago
components/device-mgt-extensions/io.entgra.device.mgt.core.device.mgt.extensions.device.organization.api/src/main/java/io/entgra/device/mgt/core/device/mgt/extensions/device/organization/api/DeviceOrganizationMgtServiceImpl.java Outdated
}
@GET
@Path("roots-children")
rajitha commented 9 months ago
Collaborator

Better to change the context path into /roots/children

Better to change the context path into /roots/children
isuri marked this conversation as resolved
isuri added 1 commit 9 months ago
tcdlpds merged commit 3092737268 into master 9 months ago

Reviewers

tcdlpds was requested for review 9 months ago
The pull request has been merged as 3092737268.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
tcdlpds
Labels
Apply labels
Clear labels
pending upstream
PR depends on pending upstream dependency
No Label pending upstream
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: community/device-mgt-core#353
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'isuri/device-mgt-core:deviceOrgImprovements'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes