Add change processing #9
Conversation
There was a problem hiding this comment.
Let's take the chance for reformatting the affected files according to CiviCRM code style, see #10 for introducing code quality tools.
| @@ -21,7 +23,8 @@ | |||
| * | |||
| * @package Civi\Api4 | |||
| */ | |||
| class Message { | |||
| class Message | |||
There was a problem hiding this comment.
I'd suggest naming it ChangeMessage.
There was a problem hiding this comment.
not sure, moved this to #11
There was a problem hiding this comment.
disagree, but let's discuss
| foreach ($changes as $change) { | ||
| $this->applyChange($change); | ||
| $change_processor = new \Civi\Share\ChangeProcessingEvent($change['id'], $local_node_id, $change); |
There was a problem hiding this comment.
Let's name the variable $changeProcessingEvent or just $event.
| $this->applyChange($change); | ||
| $change_processor = new \Civi\Share\ChangeProcessingEvent($change['id'], $local_node_id, $change); | ||
| try { | ||
| \Civi::dispatcher()->dispatch('de.systopia.change.process', $change_processor); |
There was a problem hiding this comment.
Let's define the event name in a constant on the event class and rename it to match the CiviCRM event naming schema, e. g. civi.share.change.process.
| $changes = civicrm_api4('ShareChange', 'get', [ | ||
| 'where' => [ | ||
| ['id', 'IN', $this->change_ids], | ||
| ['status', 'IN', ShareChange::ACTIVE_STATUS], | ||
| ], | ||
| 'checkPermissions' => TRUE, | ||
| ]); |
There was a problem hiding this comment.
Let's use OOP style for executing API4 actions.
| ->addValue('status', \Civi\Api4\ShareChange::STATUS_PENDING) | ||
| ->addValue('change_type', 'civishare.change.test') | ||
| ->addValue('status', 'PENDING') |
There was a problem hiding this comment.
status is set twice, I suppose the second one is superfluous.
| @@ -135,27 +138,96 @@ public function send($local_node_id = null) | |||
| * | |||
| * @return void | |||
| */ | |||
| public function processOnNode($local_node_id) | |||
| public function processChanges($local_node_id) | |||
There was a problem hiding this comment.
Is $local_node_id supposed to be the source node? Is this the internal or external ID of the node?
| @@ -103,11 +106,11 @@ public function send($local_node_id = null) | |||
| foreach ($peerings as $peered_node) { | |||
There was a problem hiding this comment.
Let's rename $peered_node to $peering, as it is not holding the node itself.
There was a problem hiding this comment.
I'd actually suggest splitting this class into one for messages to be sent and one for received messages.
There was a problem hiding this comment.
disagree, but let's discuss
| @@ -135,27 +138,96 @@ public function send($local_node_id = null) | |||
| * | |||
| * @return void | |||
| */ | |||
| public function processOnNode($local_node_id) | |||
| public function processChanges($local_node_id) | |||
| { | |||
| $lock = \Civi::lockManager()->acquire('data.civishare.changes'); // is 'data' the right type? | |||
There was a problem hiding this comment.
The comment should be a TODO on a separate line above.
| /** @var array change data */ | ||
| protected ?array $change_data = null; |
There was a problem hiding this comment.
This could be a separate class Change (or maybe two for changes to be sent and received changes) with defined properties and methods for things currently done by the Message class, e.g. markAllChanges(), processChanges(), setChangeStatus(), etc.
There was a problem hiding this comment.
Disagree, but let's discuss
adds a symfony-driven system for change processing