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.

Sign up for GitHub

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 an error notification in UI when given date/period combination is invalid #16840

Merged
merged 14 commits into from Dec 17, 2020
Merged

Show an error notification in UI when given date/period combination is invalid #16840

merged 14 commits into from Dec 17, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Nov 30, 2020

Description:

fixes #16342

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

@sgiehl sgiehl added the Needs Review PRs that need a code review label Nov 30, 2020
@sgiehl sgiehl added this to the 4.1.0 milestone Nov 30, 2020
diosmosis
diosmosis requested changes Dec 1, 2020
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.

left one comment, otherwise code looks good and seems to work locally

I'm assuming the content screenshot changes are due to the getSearchParam change, though I couldn't figure out where exactly it would use the improper date before.

@diosmosis
Copy link
Member

@sgiehl some angularjs tests are failing

@sgiehl
Copy link
Member Author

sgiehl commented Dec 10, 2020

@diosmosis there was a problem with broadcast.getParamValue().
That method actually never worked correctly for some cases. It was searching the url for param + '=' or param + '[]=', which was actually not correct in some cases.

As soon as the request param name was included in another param the method actually returned the wrong value.
Let's assume a url like ?myparam=1&param=2. Calling broadcast.getParamValue('param') would actually have returned 1 instead of 2. That behavior actually caused the test failures as subcategory contains category and so the wrong value was returned.

I've changed the method so it hopefully should work correct now. Let's see if all tests are still passing...

diosmosis
diosmosis reviewed Dec 10, 2020
View reviewed changes
plugins/CoreHome/angularjs/comparisons/comparisons.service.spec.js Outdated
@@ -260,6 +261,7 @@
it('should return false if comparison is disabled for the page', function () {
$location.search('category=MyModule2&subcategory=disabledPage2&date=2018-01-02&period=day&segment=&compareDates[]=2018-03-04&comparePeriods[]=week&compareSegments[]=comparedsegment');
$rootScope.$apply();
console.log($window.location.href);
Copy link
Member

Choose a reason for hiding this comment

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

fyi, debug code

@diosmosis
Copy link
Member

@sgiehl given the method was buggy, do you think it might help to have angular unit tests for this method in broadcast?

@sgiehl
Copy link
Member Author

sgiehl commented Dec 14, 2020

@diosmosis added some simple tests

@diosmosis
Copy link
Member

@sgiehl one screenshot test failure needs to be looked at: https://builds-artifacts.matomo.org/matomo-org/matomo/invaliddate/44416/PeriodSelector_invalid.png then it's good to merge i'd say

@sgiehl
Copy link
Member Author

sgiehl commented Dec 15, 2020

@diosmosis the test should pass now

@diosmosis
Copy link
Member

@sgiehl there appear to be two more comparison related test failures

@sgiehl
Copy link
Member Author

sgiehl commented Dec 16, 2020

@diosmosis ok. hope this time everything works again 🙈

@diosmosis diosmosis merged commit 9e60946 into 4.x-dev Dec 17, 2020
@diosmosis diosmosis deleted the invaliddate branch December 17, 2020 03:36
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
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

Error Call to undefined method Piwik\\DataTable\\Simple::getComparisons()
2 participants
@sgiehl @diosmosis