Sharing feature #566
Sharing feature #566
Conversation
franklma
commented
Nov 25, 2020
franklma
commented
- Sharing window in the portal
- Dashboard list window
- Filter list window
- Migration solution for usermetadata table
| private String createdBy; | ||
| private String updatedTime; | ||
| private String accessType; | ||
| private boolean canShare; |
There was a problem hiding this comment.
Please consider renaming to shareable, editable, viewable. Makes the getter/setter readable
|
|
||
| @Inject | ||
| public AuthorizationServiceImpl(final WorkspaceService workspaceService) { | ||
| public AuthorizationServiceImpl(final WorkspaceService workspaceService, UserMetadataService userMetadataService) { |
There was a problem hiding this comment.
Please make UserMetadataService final as well.
| @@ -98,6 +101,14 @@ public SecurityService getSecurityService() { | |||
| return securityService; | |||
| } | |||
|
|
|||
| public AuthorizationService getAuthorizationService() { | |||
There was a problem hiding this comment.
Why isnt the authorizationService injected?
| Group userAsGroup = getSecurityService().getGroupByName(userName); | ||
| Map<Group, AccessType> groupAccessMap; | ||
| Integer selectedItemId = summaryType.getId(); | ||
| if (summaryType instanceof ProcessSummaryType) { |
There was a problem hiding this comment.
Please dont use instanceOf.
There was a problem hiding this comment.
@karthik-balasubramanian I think in this context, instanceof is simply used for type checking to verify allowable actions for the object of interest. Could you suggest an alternative to do that? We can introduce some kind of enum-based typing but instanceof seems to be the most efficient (https://stackoverflow.com/revisions/26514984/10).
There was a problem hiding this comment.
Using instanceOf operator is not object oriented programming. We should create an interface that has an abstract method, say getAllowableAction that returns the groupAccessMap. The summary type can implement that interface and each summary type will provide the appropriate access type.
Also we dont need to do premature optimization. One level of indirection is not going to add a lot of performance impact here.
| Usermetadata findById(Integer id); | ||
| // | ||
| // @Query("SELECT um FROM Usermetadata WHERE um.id = ?1 FOR UPDATE") | ||
| // @org.springframework.data.jpa.repository.QueryHints(value = { |
There was a problem hiding this comment.
Please remove this commented out query
|
Please add unit tests for the Controller, Service classes etc. |
| } | ||
|
|
||
| @Override | ||
| public void saveLogAccessType(Integer logId, String groupRowGuid, AccessType accessType, boolean shareUserMetadata) { | ||
|
|
||
| if (!accessType.equals(AccessType.NONE)) { |
There was a problem hiding this comment.
Please change to if(!AccessType.NONE.equals(accessType)) to avoid NPE
| @Override | ||
| public void saveUserMetadtarAccessType(Integer userMetadataId, String groupRowGuid, AccessType accessType) { | ||
|
|
||
| if (!accessType.equals(AccessType.NONE)) { |
There was a problem hiding this comment.
same as above
| createGroupLog(group, log, hasRead, hasWrite, hasOwnership); | ||
|
|
||
| Folder parentFolder = log.getFolder(); | ||
| while (parentFolder != null && parentFolder.getId() > 0) { |
There was a problem hiding this comment.
Maybe change this to a recursive call instead of using a while loop
There was a problem hiding this comment.
I'm satisfied that this is good enough to merge.
The need to create new classes like Artifact and AccessRights isn't so much a problem of code duplication as indicative that our existing business object model is inadequate and forcing people to work around it. Until we can find the time to fix the foundations, this sort of workaround is as clean a way as we can manage.
Minor quibble: it'd be nice to standardize on either Usermetadata or UserMetadata.
