OBGM-493 Unable to assign user's role for specific location manually#4111
OBGM-493 Unable to assign user's role for specific location manually#4111kchelstowski merged 1 commit intofeature/upgrade-to-grails-3.3.10from
Conversation
| import javax.imageio.ImageIO as IIO | ||
| import javax.swing.* | ||
| import grails.gorm.transactions.Transactional | ||
| import java.util.List |
There was a problem hiding this comment.
This error
GroovyCastException: Cannot cast object '[Requestor]' with class 'java.util.ArrayList' to class 'java.awt.List' due to: groovy.lang.GroovyRuntimeException: Could not find matching constructor for: java.awt.List(org.pih.warehouse.core.Role)
was caused by the fact that we used List from java.awt instead of java.util
There was a problem hiding this comment.
@alannadolny You should remove the import java.awt.* instead of adding import java.util.List
There was a problem hiding this comment.
Yeah there's no reason for us to have the wildcard imports (javax.swing.* and java.awt.*). Looks like it was from the original migration.
@kchelstowski This should not have been merged without removing that import. So please make sure there are at least two approvals (or @awalkowiak or @jmiranda has a chance to review before merging).
There was a problem hiding this comment.
@jmiranda I have not noticed this has not been removed. The reason I merged it alone, was that in the morning Katarzyna had problem with obdev3 not working and we thought a merge could help, before Artur is online (he starts work later than us). I wouldn't have merged it if it hadn't been the case. Sorry about that.
jmiranda
left a comment
jmiranda
left a comment
There was a problem hiding this comment.
Should not have been merged.
| import javax.imageio.ImageIO as IIO | ||
| import javax.swing.* | ||
| import grails.gorm.transactions.Transactional | ||
| import java.util.List |
There was a problem hiding this comment.
Yeah there's no reason for us to have the wildcard imports (javax.swing.* and java.awt.*). Looks like it was from the original migration.
@kchelstowski This should not have been merged without removing that import. So please make sure there are at least two approvals (or @awalkowiak or @jmiranda has a chance to review before merging).
| List<Role> roles = params.list("role.id").collect { roleId -> Role.get(roleId) } | ||
|
|
||
| // Update existing role | ||
| LocationRole locationRole = LocationRole.get(params.id) |
There was a problem hiding this comment.
This is probably ok for now, but we should be careful about how we refactor these actions. I don't actually know what the original code is trying to do (I'm to blame as the original author) so adding a method with a bunch of arguments might not be the right solution.
In order to refactor properly
- We need to be very thoughtful about what the interface / API is supposed to be doing here. For example, are we trying to do multiple actions? If so, maybe we need multiple methods.
- Maybe think about passing each param separately and loading objects in the service that way we're doing all reading / writing in the same Hibernate session.
There was a problem hiding this comment.
We also need to consider the return value for new service methods. Again this is why I'd prefer to leave controllers alone unless we have very straightforward refactorings.
There was a problem hiding this comment.
This import was removed in the PR mentioned above.
4 participants
No description provided.