PIR Stabilization: Extract Operation Building from Processor

[aataraxiaa]

Task/Issue URL: https://app.asana.com/0/1199230911884351/1207175016813446/f
Tech Design URL: https://app.asana.com/0/0/1207199754528648/f
CC:

Description:
This PR includes all changes relating to this tech design sub-task. Specifically:

1. Extracts DataBrokerOperationsCollection building to a dedicated type, and adds tests
2. Encapsulates all dependencies needed to create operations into a dedicated type
3. Uses DataBrokerOperationsBuilder to build operations in the ‘Processor'
4. Renames DataBrokerOperationsCollection to DataBrokerOperation
5. Renames original DataBrokerOperation to DataBrokerJob, and renames related types
6. Renames BrokerOperationData and related types to BrokerJobData

Note: There is a lot of renaming changes in this PR. To mitigate this, the renaming changes are all grouped in dedicated commits. Therefore, to review this PR, it’s best to review commit-by-commit. This way you can focus on the commits which are not purely renaming-related.

Steps to test this PR:

1. Launch the browser
2. Open PIR and initiate a new scan (i.e new profile or edit existing profile)
3. Ensure the scan runs and results are received

—

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

[github-actions]

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against ee49467

[aataraxiaa]

CC @Bunn - this is the PR we chatted about on MM.

[aataraxiaa]

Some feedback here re: naming of Builder. I don’t feel too strongly, but will wait for any more input here from reviewers before making any changes.

[THISISDINOSAUR]

I also have little opinion on it, except for avoiding initializer since it's obviously a thing already

[aataraxiaa]

Renamed to DataBrokerOperationsCreator

[THISISDINOSAUR]

Massive props for seperating out different commits, it makes it much easier to review!

[THISISDINOSAUR]

I also have little opinion on it, except for avoiding initializer since it's obviously a thing already

[THISISDINOSAUR]

I can't quite come up with anything I'm completely happy with, but it would be good to distinguish between the types of parameters here (a/ things that dictate the type of operations returned and b/ things the operations depend on), how about:

func operations(forOperationType operationType: OperationType,
                    withPriorityDate priorityDate: Date?,
                    showWebView: Bool,
                    operationDependencies: OperationDependencies) throws -> [DataBrokerOperationsCollection]

Although if you accused me of never getting over Objective-C from this suggestion it would be hard to rebuke

[aataraxiaa]

Nice idea (even if influenced by Obj-C 😄). I’ll make the suggested changes.

[THISISDINOSAUR]

What's the plan for error propagation?

[aataraxiaa]

Good question! My initial thinking here was:

• We are currently not utilizing the error propagation (we were simply firing a pixel, while also injecting the pixel handler into DataBrokerOperation. So I just changed to fire the pixel directly in the DataBrokerOperation.
• Based on above, I didn’t want to carry-over code from the old implementation if there was no explicit use for it.
• I considered error propagation, and decided that not including it right now (as we don’t use it), while also refactoring in a way that it’s easy to add later, was the way to go.
• However , saying all that, I think we can conclusively say that we do want error propagation as some point, and so it’s not an over-optimization. As such, I can re-add it, and we can decide how to use it later.

Edit: After looking more at the code, and thinking about this, I don’t think we should propagate at the moment. This is because we also ‘handle’ errors in the barrier block of the processor. As follows:

operationQueue.addBarrierBlock {
            let operationErrors = operations.compactMap { $0.error }
            let errorCollection = operationErrors.count != 0 ? DataBrokerProtectionSchedulerErrorCollection(operationErrors: operationErrors) : nil
            completion(errorCollection)
        }

Not sure this is what we want, but until we know it isn’t, I don’t think we need an explicit propagation from individual operations.

What do you think?

[THISISDINOSAUR]

I think we at least need a plan for it, do you have any ideas on what the best way to add it would be? It might be easier to reason about in the context of the next PR

I think we do need to make sure if the function is interrupted it still returns scan errors correctly

[aataraxiaa]

Cool, yup, agree that we need to ensure that interrupted operation queues should still return errors, and also that this will be easier to reason about with the subsequent ‘QueueManager’ work. I’ll fix Swiftlint errors and merge this.

[THISISDINOSAUR]

LGTM!

[Bunn]

LGTM, just note that the failing tests are failing for an actual reason:

Error: cannot find type 'SchedulerConfig' in scope
final class MockSchedulerConfig: SchedulerConfig {

Please take a look at this before merging