Page MenuHomePhabricator

[lib] refactor sending reports actions for using operations
ClosedPublic

Authored by kamil on Tue, May 23, 1:21 AM.

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.Tue, May 23, 1:56 AM
tomek requested changes to this revision.Wed, May 24, 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.Wed, May 24, 2:48 AM
kamil requested review of this revision.Thu, May 25, 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.Thu, May 25, 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),
);