Tools module functionality #2389
Conversation
…n script) - distinguishing the Tools listing logic for administrators and the other users - replacing PermissionManager usage with UserRepository when appropriate to avoid exceptions when there is no security enabled
| @Column(name = "description") | ||
| private String description; | ||
| @Column(name = "is_enabled") | ||
| private Boolean isEnabled; |
There was a problem hiding this comment.
The property name should be renamed to 'enabled'
| import org.ohdsi.webapi.tool.dto.ToolDTO; | ||
| import org.springframework.stereotype.Controller; | ||
|
|
||
| import javax.ws.rs.*; |
There was a problem hiding this comment.
Imports with * should be eliminated
| import java.util.Optional; | ||
|
|
||
| @Component | ||
| public class ToolConvertor extends AbstractDaoService { |
There was a problem hiding this comment.
I don't think we need a separate class, to be moved to ToolServiceImpl
| return tool; | ||
| } | ||
|
|
||
| private void setCreationDetails(Tool tool, Instant currentInstant) { |
There was a problem hiding this comment.
I don't think we need two separate methods
| import java.text.SimpleDateFormat; | ||
| import java.util.Date; | ||
|
|
||
| public class DateUtils { |
There was a problem hiding this comment.
The purpose of this class is unclear SimpleDateFormat usage should be sufficient
| @@ -0,0 +1,18 @@ | |||
| CREATE TABLE IF NOT EXISTS ${ohdsiSchema}.tool ( | |||
There was a problem hiding this comment.
We can combine all migration scripts into one if necessary
There was a problem hiding this comment.
I'd prefer if you did this
There was a problem hiding this comment.
In addition, please mark these migrations attached to V2.15
|
Hello Everyone. Once the migration scripts are combined and the changes @alex-odysseus requests have been implemented, I can pull down the branches to check the functionality. |
…:put && tool:*:delete' permissions
…or renaming and imports notation
| put("tool:put", "Update a Tool"); | ||
| put("tool:post", "Create a Tool"); |
There was a problem hiding this comment.
This won't work: what will happen is that when the Tool asset is deleted, all these permissions will be deleted and everyone will lose the ability to create and update a tool.
This is an example of the complication of the permission implementation we have in webapi that we'll want to resolve in 3.x or 4.x.
For details on how the non-entity specific permissions caused issues, you can refer to this issue: #2412.
Essentially, the only permissions that go into PermissionSchema are those with entity-id context. The general ones can't be there because deleting one asset will delete the entire (non-asset specific) set of permissions from everyone.
There was a problem hiding this comment.
@alex-odysseus , please provide update to this issue.
Addressing #2330