Skip to content
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

Allow users to see their time statistics #4028

Merged
merged 20 commits into from
May 23, 2019
Merged

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Apr 18, 2019

This PR enables the time statistics for non-admin users. If the user is not an admin, the view just displays the logged in user's time statistics.
Currently, the view can only be opened at <base-url>/reports/timetracking if you are not an admin.
There needs to be a link added to this view e.g. in the navbar.

URL of deployed dev instance (used for testing):

Steps to test:

  • Log in as a non-admin user.
  • Open <base-url>/reports/timetracking.
  • The view should instantly display your statistics as the logged in user is chosen as a default user.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

@fm3 @youri-k Can you please do the backend work for this?
As far as I know, the request for time statistics needs to be adjusted so that the every logged in user can see his statistics 🙂

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Apr 18, 2019

TODO-frontend

  • make the statistics view accessible via UI/Link
  • fix the flow bug

@youri-k
Copy link
Contributor

youri-k commented May 2, 2019

@fm3 I just pushed a version which seems to be working. Would you be so kind and double check especially that we're not introducing data leaks. Thx

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend LGTM + tested :)

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Could you please review the frontend changes :)?

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, the code itself looks good! However, the lab told us to do other improvements on the time statistics before releasing it to the hiwis (see #4076). So, this PR is blocked by that, too. I'd suggest to do the broader improvements in this PR, as well. Let's talk about at the sprint meeting tomorrow.

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented May 9, 2019

Frontend TODOs

  • display task id instead of annotation id.
  • remove duplicated data as this results in a second line mentioned as the second point in the discuss post.
  • show date instead of time and when time is show use 24 hour instead of am/pm.
    hint:
 hAxis: {
   format: "hh/M/d/yy",
}
  • Do not repeat dates on the x axis - Can't find a fix
  • Refine the tooltip

@MichaelBuessemeyer
Copy link
Contributor Author

  • remove "duplicated" data as this results in a second line mentioned as the second point in the discuss post.

When the timestamps of an ending of a task and the beginning of another take are very close, the second element might be wrapped into another row.
I cannot find any posts/fixes about this matter online.

  • Do not repeat dates on the x axis - Can't find a fix

There were posts but none of the suggested fixed worked :/

@philippotto Could you please review the changes for the first time 🙂 ?

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The code looks good 👍 Let's wait for the lab feedback now.

render() {
const columns = [
{ id: "AnnotationId", type: "string" },
// This label columns is somehow needed to make the custom tooltip work.
{ type: "string", id: "empty label" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... Out of curiosity, how did you find that workaround? Was it posted somewhere? If so, you could add the link so that we can follow-up with this later (maybe this will be fixed in another version of the plotting lib).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual hint was here https://developers.google.com/chart/interactive/docs/gallery/timeline#customizing-tooltips:

Hovering over a bar brings up a tooltip with the text defined in the third column. In this chart, we have to set the second column to dummy values (here, null) so that our tooltips can exist in the third column.

Imo This does not make any sense, but it finally worked after reading this 🙂.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Can you add the link as a comment to the code? :)

@philippotto philippotto changed the title [WIP] Allow users to see their time statistics [Blocked] Allow users to see their time statistics May 17, 2019
@MichaelBuessemeyer MichaelBuessemeyer changed the title [Blocked] Allow users to see their time statistics [WIP] Allow users to see their time statistics May 17, 2019
@philippotto philippotto changed the title [WIP] Allow users to see their time statistics Allow users to see their time statistics May 21, 2019
@philippotto
Copy link
Member

@MichaelBuessemeyer Could you please remove the "time tracking link" for ordinary users (so that they won't be able to see the view) and then get this PR ready to merge? The lab wants the UX improvements first and in the second round, we can make it available to all users.

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend LGTM

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@fm3 fm3 merged commit b224b91 into master May 23, 2019
@fm3 fm3 deleted the allow-users-to-see-time-stats branch May 23, 2019 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to see their own time statistics
4 participants