OBGM-514 Make the quartz monitor render properly + add security rules #4420
OBGM-514 Make the quartz monitor render properly + add security rules #4420awalkowiak merged 5 commits intofeature/upgrade-to-grails-3.3.10from
Conversation
kchelstowski
requested review from
alannadolny,
awalkowiak,
drodzewicz and
jmiranda
alannadolny
left a comment
alannadolny
left a comment
There was a problem hiding this comment.
Great finding on the implementation -> compile!
| (superuserActions[controllerName]?.contains("*") | ||
| || superuserControllers?.contains(controllerName) | ||
| || superuserActions[controllerName]?.contains(actionName) | ||
| || superuserActions['*'].any { | ||
| actionName?.startsWith(it) | ||
| }) |
There was a problem hiding this comment.
hmmm, do we have a styling rule about the placing of the logical operators?
I mean whether we should place them in the way you did, or:
| (superuserActions[controllerName]?.contains("*") | |
| || superuserControllers?.contains(controllerName) | |
| || superuserActions[controllerName]?.contains(actionName) | |
| || superuserActions['*'].any { | |
| actionName?.startsWith(it) | |
| }) | |
| (superuserActions[controllerName]?.contains("*") || | |
| superuserControllers?.contains(controllerName) || | |
| superuserActions[controllerName]?.contains(actionName) || | |
| superuserActions['*'].any { | |
| actionName?.startsWith(it) | |
| }) |
There was a problem hiding this comment.
According to Java coding conventions, a break should happen before the operator: https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html
I think I've raised this topic recently in one of the PRs, that we should discuss it and document it in our coding conventions.
| || superuserActions['*'].any { | ||
| actionName?.startsWith(it) | ||
| }) |
There was a problem hiding this comment.
| || superuserActions['*'].any { | |
| actionName?.startsWith(it) | |
| }) | |
| || superuserActions['*'].any(actionName?.startsWith(it)) | |
| ) |
I think we can make it a little more compact without having to use { ... } but it doesn't really matter I am fine with the way it looks.
There was a problem hiding this comment.
I agree, I have not realized that, because this code has been already there, the only thing I've introduced here is:
superuserActions[controllerName]?.contains("*")I could change it
There was a problem hiding this comment.
@drodzewicz I've applied your suggestion, but in fact, it should not be any(...), but any{...}.
There was a problem hiding this comment.
by saying so I'm starting to strongly agree with @awalkowiak that applying suggestions blindly via github sucks.
awalkowiak
changed the base branch from
feature/upgrade-to-grails-3.3.10
to
release/0.9.0-RC
awalkowiak
changed the base branch from
release/0.9.0
to
feature/upgrade-to-grails-3.3.10
5 participants
No description provided.