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

Jump to bottom

Show report generated time when date range includes today #17506

Merged
merged 11 commits into from May 27, 2021
Merged

Show report generated time when date range includes today #17506

merged 11 commits into from May 27, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Apr 28, 2021

Description:

fixes #14977

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 28, 2021
@flamisz flamisz self-assigned this Apr 28, 2021
@flamisz flamisz marked this pull request as ready for review May 2, 2021 23:36
@flamisz flamisz added the Needs Review PRs that need a code review label May 2, 2021
@flamisz flamisz added this to the 4.3.0 milestone May 2, 2021
@flamisz flamisz removed the Needs Review PRs that need a code review label May 3, 2021
@flamisz flamisz marked this pull request as draft May 3, 2021 04:24
@flamisz flamisz marked this pull request as ready for review May 4, 2021 01:29
@flamisz flamisz added the Needs Review PRs that need a code review label May 4, 2021
@flamisz
Copy link
Contributor Author

flamisz commented May 4, 2021

Note:
I wanted to show the generated time for the evolution graphs as well. I found one way, but then it modified the API results' JSON (added the generated time as metadata). I tried a couple of ways to solve it (see the commit history), but couldn't find any "clean" way I liked.
I recommend merge it as it is (of course if the PR is ok) and if we want, we can create a new issue for showing the generated time for evolution graphs. I spent too much time on this for now.

@flamisz
Copy link
Contributor Author

flamisz commented May 12, 2021

Hi @mattab and @tsteur can you please check this and my previous ⬆️ comment? Should I spend more time on or we'd like to merge it as it is for now?

@mattab
Copy link
Member

mattab commented May 14, 2021

@flamisz

I recommend merge it as it is (of course if the PR is ok) and if we want, we can create a new issue for showing the generated time for evolution graphs.

Absolutely, sounds good this way 👍

Can you wait after we released 4.3.0 (so it can be tested in 4.4.0 beta) to merge?

diosmosis
diosmosis approved these changes May 14, 2021
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

code looks good to me and change works locally

@mattab mattab modified the milestones: 4.3.0, 4.4.0 May 26, 2021
@diosmosis diosmosis merged commit 14ffece into 4.x-dev May 27, 2021
@diosmosis diosmosis deleted the 14977-show-report-generated-time branch May 27, 2021 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis approved these changes

Assignees

@flamisz flamisz

Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

Show more clearly the freshness of reports when looking at Today or This week
3 participants
@flamisz @mattab @diosmosis