-
Notifications
You must be signed in to change notification settings - Fork 24
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
Timetracking Overview Improvements #7789
Conversation
@@ -2016,6 +2016,7 @@ export async function getTimeTrackingForUserSummedPerAnnotation( | |||
if (annotationTypes != null) params.append("annotationTypes", annotationTypes); | |||
if (projectIds != null && projectIds.length > 0) | |||
params.append("projectIds", projectIds.join(",")); | |||
params.append("annotationStates", "Active,Finished"); |
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 is just a prerequisite to show the annotations state in the table and in the CSV, and to filter it, right?
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.
Right, only for the filtering at this time. I wrote this before I learned that the priority is lower for now. I guess we can use it in the frontend in a later iteration, or remove it again if we decide we don’t want to go for this option :)
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.
I took a look at the frontend code and tested the code and I think both looks good :) thank you!
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.
The backend code also looks good to me 👍
@fm3, did you take measurements on the speed up of the summedByAnnotationForUser
query? If you still have them, could you please add them to the PR for documentation purposes? 🙏
Please note that I did not do any testing and only looked at the code as @dieknolle3333 already tested the changes :)
conf/webknossos.latest.routes
Outdated
GET /time/user/:userId/spans controllers.TimeController.timeSpansOfUser(userId: String, start: Long, end: Long, annotationTypes: String, projectIds: Option[String]) | ||
GET /time/user/:userId/summedByAnnotation controllers.TimeController.timeSummedByAnnotationForUser(userId: String, start: Long, end: Long, annotationTypes: String, projectIds: Option[String]) | ||
GET /time/overview controllers.TimeController.timeOverview(start: Long, end: Long, annotationTypes: String, teamIds: Option[String], projectIds: Option[String]) | ||
GET /time/user/:userId/spans controllers.TimeController.timeSpansOfUser(userId: String, start: Long, end: Long, annotationTypes: String, projectIds: Option[String], annotationStates: String) | ||
GET /time/user/:userId/summedByAnnotation controllers.TimeController.timeSummedByAnnotationForUser(userId: String, start: Long, end: Long, annotationTypes: String, projectIds: Option[String], annotationStates: String) | ||
GET /time/overview controllers.TimeController.timeOverview(start: Long, end: Long, annotationTypes: String, teamIds: Option[String], projectIds: Option[String], annotationStates: String) |
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.
Isn't the order of parameters here important? One common rule is that Optional Parameters should be last in param order. Does this apply here as well (at least as a style)? I guess play forwards the projectIds
as Some(null)
automatically in case the param is absent. Thus, the order is likely irrelevant.
But looking at the file, in most cases the option types are last in order.
But as this is likely not a syntax error / error in the code, also feel free to ignore this comment :)
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.
I think the order isn’t relevant here for the compiler, as this is, so to speak, always called by name. If an optional parameter is omitted from a request query, it is auto-filled with None (different from Some(null). In fact we want to avoid null wherever possible, since the type-checker does not deal with it).
However, you are right; at least for style, I’ll move the new parameter so that the optional ones stay last.
Contributes to #7788 by fixing
Not included for now:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
webknossos/webknossos/client) if relevant API parts change