FIX: Dismiss unread respects tracked query param (PR #10714)

GitHub

The title of this pull request changed from “WIP: ‘dismiss…’ respectes tracked query param” to "FIX: Dismiss unread respects tracked query param

If the filter query param is “tracked” and you call refresh, then the query param is removed. I added this option, so that when dismissing unread with “tracked” set, it does not get unset.

in this case I think @eviltrout prefers to go with { resetParams: true }.

It makes the code mor extensible in the future and easier to follow when read, eg:

this.refresh(true); // what does true means here?
this.refresh({ resetParams: true }); // very clear what true means here
      topic_ids: topics.mapBy("id"),

I think that’s slightly annoying to have tracked as an arg after options, it’s generally the last arg if possible, can’t we put tracked in options ?

Good call, I’ll move it in.

Actually @jjaffeux I don’t think I want to move it into options. Both bulkOperation and bulkOperationByFilter need to know about “tracked”, and bulkOperation does not have options as a parameter. The way these functions are called also makes me want to keep it as a separate parameter.

This shouldn’t be necessary. Why can’t the below code use this.afterRefresh?

This seems a bit weird to me. You are querying the topic list, then after it’s loaded you load it again if there are query parameters? Couldn’t you load it once with the query parameters and avoid all this?

This is the tricky part. Topic tracking state needs the topic list without query parameters. If query params are present, then fetch the topic list with query params for the list model. It’s kinda hacky, but the topic tracking state needs to sync to the un-query-param’d topic list here https://github.com/discourse/discourse/blob/eb08c5453b00674ad39319a10c2fa461c607669c/app/assets/javascripts/discourse/app/controllers/discovery/topics.js#L119.

I explain this in the comment above.

I don’t get it either. I couldn’t do this.afterRefresh, but maybe I am insane. Ill go try again.

If the topic list is fetched with query params, and topic tracking state is synced to it, a hard refresh is needed to fix the topic tracking state.

I have to say I don’t really like it. Does this mean any time there are query parameters there will be two ajax requests on the server every time you view the topic list?

@eviltrout No, this is only when refresh is called, after dismissing new topics when there are query params.

Furthermore, all query params are cleared when refresh is called, except for params that are explicitly skipped (passed into the refresh function. 2 ajax calls will only ever happen when dismissing new and ?f= or ?filter= is present.

I agree with you it’s smelly… But it would be so much more work messing with topic tracking state to avoid this.

Oops I always click resolve conversation when I have a comment so it doesn’t go through. This is cool. I wanted to mention refresh probably also happens if you click the logo, but that is not as common as navigating pages.