@sgiehl opened this Pull Request on November 30th 2020 Member

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
@diosmosis commented on December 10th 2020 Member

@sgiehl some angularjs tests are failing

@sgiehl commented on December 10th 2020 Member

@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 commented on December 10th 2020 Member

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

@sgiehl commented on December 14th 2020 Member

@diosmosis added some simple tests

@diosmosis commented on December 14th 2020 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 commented on December 15th 2020 Member

@diosmosis the test should pass now

@diosmosis commented on December 16th 2020 Member

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

@sgiehl commented on December 16th 2020 Member

@diosmosis ok. hope this time everything works again 🙈

This Pull Request was closed on December 17th 2020
Powered by GitHub Issue Mirror