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 #15575 (comment) which may need to be fixed in order to test this issue :)

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

@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.

👍 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)

@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 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 😄

@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 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.

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