sentry icon indicating copy to clipboard operation
sentry copied to clipboard

ref(grouping): Remove hierarchical code from `_save_aggregate_new`

Open lobsterkatie opened this issue 1 year ago • 3 comments

Since the new group assignment logic in event_manager won't apply to anyone on the mobile grouping config, we have the freedom to remove the hierarchical code required by that config. This should make further refactoring easier, since it simplifies the code quite a bit.

lobsterkatie avatar Feb 08 '24 10:02 lobsterkatie

out of curiousity, why did no tests have to change?

This PR is part of a series of refactorings in which the following has already happened:

  • A flag was created
  • A new function with identical logic to _save_aggregate (_save_aggregate_new) was created. (The same was done for find_existing_grouphash, which is called by _save_aggregate.)
  • A wrapper for _save_aggregate and _save_aggregate_new was added.
    • If the flag is off or if the project uses (or has used) the mobile grouping config (which needs the hierarchical code), we run _save_aggregate (which calls the existing find_existing_grouphash) - in other words, all of the current logic is retained.
    • If the flag is on and there's no mobile config (and therefore no need for the hierarchical code), we run _save_aggregate_new, which then calls find_existing_grouphash_new.

Now there's this PR which pulls the hierarchical code out of _save_aggregate_new and find_existing_grouphash_new.

The reason this doesn't actually change anything is that - because of the mobile config check in the wrapper - any event which would care about the now-missing hierarchical code (i.e., an event processed through the mobile config) runs through the unchanged codepath. Conversely, the only events which run through this new codepath are ones for whom the hierarchical code is irrelevant.

(FWIW, the plan is not to have it be like this forever. Once the new logic is fully rolled out, we're then going to start moving people off of the mobile config. Once they're all off, we can then get rid of the current versions of _save_aggregate and find_existing_grouphash. Also, there are more changes to come in _save_aggregate_new. Getting rid of the now-unnecessary hierarchical code is just cleanup to make future changes easier to reason about.)

lobsterkatie avatar Feb 09 '24 01:02 lobsterkatie

Going to be pretty nice to remove all of this!

nice, so much easier to read

I so agree with both of you! There are a lot of steps between here and there, but I'm excited for when it finally happens.

lobsterkatie avatar Feb 10 '24 03:02 lobsterkatie

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6cfc89e) 81.39% compared to head (3bb22e9) 81.40%. Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #64858   +/-   ##
=======================================
  Coverage   81.39%   81.40%           
=======================================
  Files        5251     5251           
  Lines      232182   232182           
  Branches    45592    45592           
=======================================
+ Hits       188994   188998    +4     
+ Misses      37301    37298    -3     
+ Partials     5887     5886    -1     

see 7 files with indirect coverage changes

codecov[bot] avatar Feb 12 '24 21:02 codecov[bot]