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

Set the previous month date to 1 when compare months #17294

Merged
merged 4 commits into from Mar 11, 2021
Merged

Set the previous month date to 1 when compare months #17294

merged 4 commits into from Mar 11, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 2, 2021 •
edited by sgiehl

Description:

fixes #17080

Review

  • Functional review done
  • 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 self-assigned this Mar 2, 2021
@flamisz flamisz added 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. labels Mar 2, 2021
@sgiehl sgiehl changed the title Det the previous month date to 1 when compare months Set the previous month date to 1 when compare months Mar 3, 2021
@sgiehl sgiehl added this to the 4.5.0 milestone Mar 3, 2021
@sgiehl
Copy link
Member

sgiehl commented Mar 9, 2021

I guess the change is fine. Seems we currently don't have any unit tests for the periods class in javascript. Might maybe be good to at least add some basic tests that covers some of the edge cases that were making trouble.
@flamisz Not sure if you already had a look at the angularjs tests. They are located with the javascript files but are named *.spec.js

@flamisz
Copy link
Contributor Author

flamisz commented Mar 10, 2021

You were right @sgiehl, the tests are important. I found one other very similar issue this way 😄 .

sgiehl
sgiehl requested changes Mar 10, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a minor suggestion for improvements. Besides that we could think about adding some test cases for invalid dates maybe, so we know how they are handled, but that does not need to be part of this PR actually.
Otherwise looks good 👍

plugins/CoreHome/angularjs/common/services/periods.spec.js Outdated Show resolved Hide resolved
@sgiehl sgiehl modified the milestones: 4.5.0, 4.3.0 Mar 11, 2021
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 11, 2021
@sgiehl sgiehl merged commit cf0c44e into 4.x-dev Mar 11, 2021
@sgiehl sgiehl deleted the 17080-report-last-day-of-month branch March 11, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees

@flamisz flamisz

Labels
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.3.0
Development

Successfully merging this pull request may close these issues.

Matomo comparing month report with same month when clicking on the last day of the month
2 participants
@flamisz @sgiehl