Page MenuHomePhabricator

[lib] refactor sending reports actions for using operations
ClosedPublic

Authored by kamil on May 23 2023, 1:21 AM.
Tags
None
Referenced Files
F1550839: D7931.id27143.diff
Mon, Apr 15, 6:54 PM
F1550838: D7931.id27135.diff
Mon, Apr 15, 6:54 PM
F1549808: D7931.diff
Mon, Apr 15, 9:47 AM
F1548972: D7931.id26839.diff
Mon, Apr 15, 3:34 AM
F1548963: D7931.id.diff
Mon, Apr 15, 3:34 AM
Unknown Object (File)
Sat, Apr 13, 3:39 AM
Unknown Object (File)
Sat, Apr 13, 3:39 AM
Unknown Object (File)
Sat, Apr 13, 3:39 AM
Subscribers

Details

Summary

Convert logic for ops approach.

Depends on D7923

Test Plan

Tests

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.May 23 2023, 1:56 AM
tomek requested changes to this revision.May 24 2023, 2:48 AM
tomek added inline comments.
lib/reducers/report-store-reducer.js
120 ↗(On Diff #26839)

What are these newReports? Why they weren't included in the original code?

This revision now requires changes to proceed.May 24 2023, 2:48 AM
kamil requested review of this revision.May 25 2023, 3:03 AM
kamil added inline comments.
lib/reducers/report-store-reducer.js
120 ↗(On Diff #26839)

What are these newReports?

Those are inconsistencies not added via action but directly passed from other reducers.

Why they weren't included in the original code?

They were, but in previous code, they're merged with the current state, see here. Here we need to distinguish between new reports and those already in state to generate proper ops. This variable was introduced in D7923.

tomek added inline comments.
lib/reducers/report-store-reducer.js
120 ↗(On Diff #26839)

Makes sense, thanks! I guess it would be less confusing if instead of using something from outside the if, we would achieve reusability by e.g. extracting common logic to a function.

This revision is now accepted and ready to land.May 25 2023, 7:48 AM

rebase

lib/reducers/report-store-reducer.js
120 ↗(On Diff #26839)

I guess it would be less confusing if instead of using something from outside the if, we would achieve reusability by e.g. extracting common logic to a function.

I am not sure what you mean exactly by extracting common logic to a function and what we will achieve. But if you suggested wrapping everything before if ... else.. to function - this will be removed with last diff connected to refactor, and the only remaining line will be

const newReports = newInconsistencies.filter(report =>
  isReportEnabled(report, state.enabledReports),
);