-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DG-29 User admin update/Reports #603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of it looks good to me. Noticed there is only one unit test file. Are all testing done manually?
grails-app/controllers/au/org/ala/volunteer/LabelController.groovy
Outdated
Show resolved
Hide resolved
grails-app/controllers/au/org/ala/volunteer/LandingPageAdminController.groovy
Outdated
Show resolved
Hide resolved
grails-app/controllers/au/org/ala/volunteer/ProjectController.groovy
Outdated
Show resolved
Hide resolved
grails-app/controllers/au/org/ala/volunteer/UserController.groovy
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just a couple of nits if you can bothered
|
||
class LabelController { | ||
|
||
static allowedMethods = [save: "POST", update: "PUT", delete: "POST"] | ||
// static allowedMethods = [saveNewLabel: "POST", updateCategory: "POST", saveCategory: "POST", createCategory: "POST", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove or fix?
def categoryId = params.long('id') | ||
def category = LabelCategory.get(categoryId) | ||
if (!category) { | ||
log.info("No category found, redirecting back to index.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Feels like debug logging not info?
* @return a Map of user activity for a given date range. | ||
*/ | ||
def getUserActivityBetweenDates(Date startDate, Date endDate) { | ||
String query = """\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you don't need to select to_timestamp(max(vt.lastView)/1000)
as it's not used?
Also this could be done with jooq for typesafe queries, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was in case Alex wanted date parameters on the existing User summary report, in the end he decided against. I'm tempted to leave it in if that requirement changes, if you're happy with it.
DG-29 User labels and Reporting
Labels
Reports