Fix exclusive lists interfering with notifications

[ShadowJonathan]

Fixes #27316

I had to rename a parameter, and add a function, to make sure this didn't become too much of a messy hack, but i hope that this is at least still a similar level of cleanliness and understanding

The only annoyance is that this is now building crutches twice, the alternative for that would be to have some enum/flag/symbol kind of return ("hide from home, show notif", "don't show notif, hide from home", :skip_home, :filter), but that's a lot more refactoring

if that's the more clean solution, prompt me, then i'll try to refactor around it

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8710bdb) 82.74% compared to head (1d494ab) 82.74%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
app/workers/feed_insert_worker.rb	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #28162   +/-   ##
=======================================
  Coverage   82.74%   82.74%           
=======================================
  Files        1038     1038           
  Lines       28221    28223    +2     
  Branches     4552     4552           
=======================================
+ Hits        23351    23354    +3     
  Misses       3784     3784           
+ Partials     1086     1085    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ClearlyClaire]

The only annoyance is that this is now building crutches twice

This could be an issue, as this part of the code is in a hot path. I think we might want to rework it so that Home, Notifications and List delivery happen in the same job unit, with a single call to build_crutches.

[ShadowJonathan]

@renchap not a lint fix, the additional code needed to wrap around

[ShadowJonathan]

@ClearlyClaire does that mean that you/someone will eventually create a PR that refactors this code, and that this PR won't move forward?

[ClearlyClaire]

@ClearlyClaire does that mean that you/someone will eventually create a PR that refactors this code, and that this PR won't move forward?

I experimented a bit, and the database schema does not lend itself well to what I was suggesting. So we probably want to keep the current split in jobs, at least for the time being, and instead look into keeping your PR but avoiding the duplicate work between filter?(:home, …) and filter_notification?.

[ShadowJonathan]

Alright, would a symbol return (:unfiltered, :skip_home, :filtered) to filter? be a good solution then?

[mjankowski]

I think this needs a rebase and CI is frozen, but also -- seems like we've been blocked for about a year on whether we can resolve the performance implications of double crutching?

@ShadowJonathan - do you want to rebase and continue to nudge this forward, or should we close?

[ShadowJonathan]

I'll take a look at this, thanks for bumping

[ShadowJonathan]

Rebased on (my 2-day-old cached local copy of) main

[ShadowJonathan]

Alright, would a symbol return (:unfiltered, :skip_home, :filtered) to filter? be a good solution then?

@ClearlyClaire this has been a year in the making, but what's your opinion on this?

[ClearlyClaire]

What about either a symbol or nil? :filtered if filtered, :skip_home if filtered for home when checking for notifications, and nil otherwise. That way, the truthy behavior evaluation stays the same, but you can still discriminate between :skip_home and :filtered in FeedInsertWorker?

[ShadowJonathan]

Didn't know that symbols were truthy and nil was falsey, but yes, that sounds perfect. I'll implement that.

[ShadowJonathan]

@ClearlyClaire @mjankowski this PR is ready for review.

[ClearlyClaire]

Ah, this is a little more awkward and involved than I anticipated, and I missed the bit about the function being called filter?; by convention, this should return a boolean… I guess having filter and filter? that just wraps around it would make sense.

[ShadowJonathan]

@ClearlyClaire applied! The PR should be much cleaner to review now ^^

[ClearlyClaire]

I wonder if there's a way to make it read less awkward, but it's not too bad, and it should fix an actual issue.

[mjankowski]

I see changes to existing specs to handle the signature change - should there be new spec for the fixed/change list scenario here?