@flamisz opened this Pull Request on May 4th 2021 Contributor

Description:

As in title.

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
@tsteur commented on May 5th 2021 Member

btw @sgiehl @flamisz not sure what you exactly meant in the comment but if the request isn't sent currently that might be because of https://github.com/matomo-org/matomo/issues/15575#issuecomment-822897559 which may need to be fixed in order to test this issue :)

@tsteur commented on May 5th 2021 Member

it might be totally unrelated though so feel free to ignore, only thought to mention it in case it's related

@flamisz commented on May 5th 2021 Contributor

@flamisz Did you test that locally? For me the XHR request is actually never sent, as the notificationId is always null. Guess it needs to be passed to markNotificationAsRead from the html template instead, as it isn't available in the controller.

@tsteur @sgiehl I realizes something is not ok with the notification, but I thought I just don't know how to use it properly. I was able to test it tho. I created permanent notification (hardcoded somewhere into a plugin) and used an API call to mark as readed. Before the PR I was able to mark as readed so it disappeared for the next request. With this modifications I needed a token to do that.

@tsteur commented on May 5th 2021 Member

👍 we'd just need to double check that we also send the token in the actual request from the notification JS otherwise it won't work basically (but it's not working there currently anyway so could also ignore it and not test it)

@sgiehl commented on May 6th 2021 Member

@flamisz while reviewing I actually saw why the notification xhr didn't work properly. I've pushed a commit to this branch to fix that. So maybe this then also fixes #15575

@flamisz commented on May 6th 2021 Contributor

@flamisz while reviewing I actually saw why the notification xhr didn't work properly. I've pushed a commit to this branch to fix that. So maybe this then also fixes #15575

thanks @sgiehl, this makes sense. I just wasn't sure if it's a bug or my lack of knowledge 😄

@sgiehl commented on May 11th 2021 Member

@flamisz I've rebased the branch and added some new UI tests to check that closing the notifications work as expected. Feel free to have a look at the UI tests and merge the PR if they look good to you.

@flamisz commented on May 11th 2021 Contributor

@flamisz I've rebased the branch and added some new UI tests to check that closing the notifications work as expected. Feel free to have a look at the UI tests and merge the PR if they look good to you.

Thanks, @sgiehl, looking good 👍
Probably wait with the merge until we have the new release.

@sgiehl commented on May 12th 2021 Member

Might maybe be worth to include that in 4.3.0 maybe, as it fixes the uncloseable notification issue. ping @tsteur

This Pull Request was closed on May 12th 2021
Powered by GitHub Issue Mirror