Add getDevicesByUserId() method to fetch registered devices - #300
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds Sequence Diagram(s)sequenceDiagram
participant DevicesApiServiceImpl
participant PushDeviceManagementService
participant deviceHandlerService
DevicesApiServiceImpl->>PushDeviceManagementService: getDevicesByUserId()
PushDeviceManagementService->>deviceHandlerService: getDevicesByUserId(userId, tenantDomain)
deviceHandlerService-->>PushDeviceManagementService: List<Device>
PushDeviceManagementService-->>DevicesApiServiceImpl: List<DeviceDTO>
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.java (1)
234-246: ⚡ Quick winReuse the existing single-device mapper to avoid duplicated mapping logic.
Line 238-Line 243 duplicates
buildDeviceDTO(Device). Reusing the existing helper keeps mapping behavior consistent and reduces drift risk.Proposed refactor
private List<DeviceDTO> buildListDeviceDTO(List<Device> deviceList) { List<DeviceDTO> deviceDTOList = new ArrayList<>(); for (Device device : deviceList) { - DeviceDTO deviceDTO = new DeviceDTO(); - deviceDTO.setDeviceId(device.getDeviceId()); - deviceDTO.setName(device.getDeviceName()); - deviceDTO.setModel(device.getDeviceModel()); - deviceDTO.setProvider(device.getProvider()); - deviceDTOList.add(deviceDTO); + deviceDTOList.add(buildDeviceDTO(device)); } return deviceDTOList; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.java` around lines 234 - 246, The buildListDeviceDTO method is duplicating device mapping logic that already exists in the buildDeviceDTO(Device) method. Replace the manual field assignments (setDeviceId, setName, setModel, setProvider) inside the for loop with a call to the existing buildDeviceDTO method for each Device in the deviceList, capturing its result and adding it to the deviceDTOList. This eliminates code duplication and ensures consistent mapping behavior across both methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.java`:
- Around line 234-246: The buildListDeviceDTO method is duplicating device
mapping logic that already exists in the buildDeviceDTO(Device) method. Replace
the manual field assignments (setDeviceId, setName, setModel, setProvider)
inside the for loop with a call to the existing buildDeviceDTO method for each
Device in the deviceList, capturing its result and adding it to the
deviceDTOList. This eliminates code duplication and ensures consistent mapping
behavior across both methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08f864dd-778e-4ee2-90e1-c4787b5c01d6
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.javacomponents/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/impl/DevicesApiServiceImpl.java
fc241bf to
2b49aa0
Compare
| deviceDTOList = buildListDeviceDTO(devices); | ||
| } catch (PushDeviceHandlerException e) { | ||
| // If no device registered for the userId, the below-mentioned error is thrown. | ||
| if (!ERROR_CODE_DEVICE_NOT_FOUND_FOR_USER_ID.getCode().equals(e.getErrorCode())) { |
There was a problem hiding this comment.
this Error doesnt come in the new getDevicesByUserId
deeff5b to
c618371
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.java (1)
236-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnsure
deviceListis not null before iteration.If
deviceHandlerService.getDevicesByUserIdreturnsnullinstead of an empty list, iterating overdeviceListwill result in aNullPointerException. Consider adding a null check to handle potential null values safely.♻️ Proposed refactor
private List<DeviceDTO> buildListDeviceDTO(List<Device> deviceList) { List<DeviceDTO> deviceDTOList = new ArrayList<>(); - for (Device device : deviceList) { - deviceDTOList.add(buildDeviceDTO(device)); + if (deviceList != null) { + for (Device device : deviceList) { + deviceDTOList.add(buildDeviceDTO(device)); + } } return deviceDTOList; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.java` around lines 236 - 243, Update buildListDeviceDTO to handle a null deviceList before the enhanced for-loop, returning the existing empty deviceDTOList when no devices are provided while preserving normal conversion through buildDeviceDTO for non-null lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.java`:
- Around line 236-243: Update buildListDeviceDTO to handle a null deviceList
before the enhanced for-loop, returning the existing empty deviceDTOList when no
devices are provided while preserving normal conversion through buildDeviceDTO
for non-null lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7529c624-eb96-4e01-bafa-2c5d129d8cfc
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/core/PushDeviceManagementService.javacomponents/org.wso2.carbon.identity.api.user.push/org.wso2.carbon.identity.rest.api.user.push.v1/src/main/java/org/wso2/carbon/identity/rest/api/user/push/v1/impl/DevicesApiServiceImpl.java
Adds a method to return all push-auth devices registered for the current user, supporting multiple device registration.
c618371 to
ec96978
Compare
|
PR builder started |
|
PR builder completed |
Adds a method to return all push-auth devices registered for the current user, supporting multiple device registration.
d451e7c to
f72f919
Compare
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/29500960466
3 participants
This pull request adds a new method to retrieve devices by user ID and refactors the code to use this new method, improving clarity and maintainability. It also introduces a helper method to build lists of device DTOs from model objects.
Device retrieval improvements:
getDevicesByUserIdmethod inPushDeviceManagementServiceto fetch all devices for the current user, with improved error handling for cases where no devices are found.DevicesApiServiceImplto use the newgetDevicesByUserIdmethod instead of the old one, ensuring consistency and leveraging the improved logic.Code modularization:
buildListDeviceDTOinPushDeviceManagementServiceto convert a list ofDeviceobjects into a list ofDeviceDTOobjects, reducing code duplication and improving readability.Related Issues