added user attribute for loan officer#30
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideExtends loan officer user attributes to include new document download and view permissions, and ensures these permissions are set when building registration attributes for loan officers. Flow diagram for loan officer registration attributes with new permissionsflowchart TD
A[buildRegistrationAttributes]
A --> B{registrationData.userRole == KEY_USER_ROLE_LOAN_OFFICER}
B -->|yes| C[getAttributesForLoanOfficer]
B -->|no| D[Return base attributes]
C --> E["attributes.put(DOCUMENT_UPLOAD_PERMISSION, true)"]
C --> F["attributes.put(DOCUMENT_DOWNLOAD_PERMISSION, true)"]
C --> G["attributes.put(DOCUMENT_FETCH_PERMISSION, true)"]
E --> H[Return attributes]
F --> H
G --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
DOCUMENT_FETCH_PERMISSIONconstant is mapped to the"document:view"value, which is slightly inconsistent with its name; consider renaming either the constant or the value so the terminology is aligned (e.g.,DOCUMENT_VIEW_PERMISSIONor"document:fetch").
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DOCUMENT_FETCH_PERMISSION` constant is mapped to the `"document:view"` value, which is slightly inconsistent with its name; consider renaming either the constant or the value so the terminology is aligned (e.g., `DOCUMENT_VIEW_PERMISSION` or `"document:fetch"`).
## Individual Comments
### Comment 1
<location path="src/main/java/com/loan_org/identity_and_access_management/util/UserAttributeFactory.java" line_range="18" />
<code_context>
public static final String DOCUMENT_UPLOAD_PERMISSION = "document:upload";
+ public static final String DOCUMENT_DOWNLOAD_PERMISSION = "document:download";
+ public static final String DOCUMENT_FETCH_PERMISSION = "document:view";
public Map<String, Object> buildRegistrationAttributes(UserRegistrationDto registrationData) {
</code_context>
<issue_to_address>
**suggestion:** Clarify the semantic difference between `download` and `view` permissions to avoid future misuse.
Since `DOCUMENT_FETCH_PERMISSION` maps to `"document:view"`, the name may imply a more generic operation than intended. If `view` and `download` are distinct capabilities, consider renaming the constant to match the permission string (e.g., `DOCUMENT_VIEW_PERMISSION`) or adjusting the permission string to match the name, to keep permission checks unambiguous across the codebase.
Suggested implementation:
```java
public static final String DOCUMENT_UPLOAD_PERMISSION = "document:upload";
public static final String DOCUMENT_DOWNLOAD_PERMISSION = "document:download";
public static final String DOCUMENT_VIEW_PERMISSION = "document:view";
```
```java
private void getAttributesForLoanOfficer(Map<String, Object> attributes) {
attributes.put(DOCUMENT_UPLOAD_PERMISSION, true);
attributes.put(DOCUMENT_DOWNLOAD_PERMISSION, true);
attributes.put(DOCUMENT_VIEW_PERMISSION, true);
}
```
1. Search the entire codebase for usages of `DOCUMENT_FETCH_PERMISSION` and update them to `DOCUMENT_VIEW_PERMISSION`.
2. If any configuration, documentation, or tests reference `DOCUMENT_FETCH_PERMISSION`, update them as well to keep naming consistent and avoid confusion between `view` and `download` capabilities.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| public static final String DOCUMENT_UPLOAD_PERMISSION = "document:upload"; | ||
| public static final String DOCUMENT_DOWNLOAD_PERMISSION = "document:download"; | ||
| public static final String DOCUMENT_FETCH_PERMISSION = "document:view"; |
There was a problem hiding this comment.
suggestion: Clarify the semantic difference between download and view permissions to avoid future misuse.
Since DOCUMENT_FETCH_PERMISSION maps to "document:view", the name may imply a more generic operation than intended. If view and download are distinct capabilities, consider renaming the constant to match the permission string (e.g., DOCUMENT_VIEW_PERMISSION) or adjusting the permission string to match the name, to keep permission checks unambiguous across the codebase.
Suggested implementation:
public static final String DOCUMENT_UPLOAD_PERMISSION = "document:upload";
public static final String DOCUMENT_DOWNLOAD_PERMISSION = "document:download";
public static final String DOCUMENT_VIEW_PERMISSION = "document:view"; private void getAttributesForLoanOfficer(Map<String, Object> attributes) {
attributes.put(DOCUMENT_UPLOAD_PERMISSION, true);
attributes.put(DOCUMENT_DOWNLOAD_PERMISSION, true);
attributes.put(DOCUMENT_VIEW_PERMISSION, true);
}- Search the entire codebase for usages of
DOCUMENT_FETCH_PERMISSIONand update them toDOCUMENT_VIEW_PERMISSION. - If any configuration, documentation, or tests reference
DOCUMENT_FETCH_PERMISSION, update them as well to keep naming consistent and avoid confusion betweenviewanddownloadcapabilities.
Summary by Sourcery
Extend loan officer user attributes with additional document access permissions.
New Features:
Enhancements: