Update/create time period entries efficiently #114
Conversation
raviks789
force-pushed
the
improve-timeperiod-storage
branch
4 times, most recently
from
44ee546
to
d43b162
Compare
raviks789
changed the title
raviks789
force-pushed
the
improve-timeperiod-storage
branch
from
d43b162
to
3c30bea
Compare
| @@ -123,6 +124,10 @@ public function loadEntry(int $scheduleId, int $id): self | |||
|
|
|||
| $recipients = []; | |||
| foreach ($members as $member) { | |||
| if (! isset($values['membership_hash'])) { | |||
| $values['membership_hash'] = (new Binary([]))->toDb($member->membership_hash, null, null); | |||
There was a problem hiding this comment.
membership_hash is nullable and this line doesn't account for that and leads to errors for me.
I think registering NULL as value is just fine. Though, remember to switch to array_key_exists above then.
Besides, don't use the behavior here, just use bin2hex and later when processing the hash again, hex2bin. There's no need to do this database vendor aware.
| $members = ScheduleMember::on(Database::get()) | ||
| ->with('timeperiod') | ||
| ->filter( | ||
| Filter::all( | ||
| Filter::equal('timeperiod.owned_by_schedule_id', $scheduleId), | ||
| Filter::equal('schedule_member.membership_hash', $membershipHash) | ||
| ) | ||
| ); | ||
|
|
||
| if ($members->count() > 0) { |
There was a problem hiding this comment.
I'd rather select from Timeperiod directly. That's the only information you're really interested in. The count of members for example, you're using it to check whether there are any members at all. That's a common mistake, I mean, to count although the count itself is of no interest. So by fetching the timeperiod directly, you already verified that at least one member's hash matches. (Given that the filter is still the same)
| 'contactgroup_id' => $id | ||
| ]); | ||
| if ($members->count() === 0) { | ||
| $binaryMemberHash = (new Binary([]))->toDb($membershipHash, null, null); |
There was a problem hiding this comment.
| $binaryMemberHash = (new Binary([]))->toDb($membershipHash, null, null); | |
| $q = ScheduleMember::on(Database::get()); | |
| $binaryMemberHash = $q->getResolver()->getBehaviors($q->getModel())->persistProperty($membershipHash, 'membership_hash'); |
This way you don't hardcode the binary behavior here and it will still work once are compatible with MySQL.
| * | ||
| * @return string | ||
| */ | ||
| public function getMembershipHashFromRecipients(int $scheduleID): string |
There was a problem hiding this comment.
Please use $scheduleId (with a lowercase d at the end) instead. addEntry does it already.
| { | ||
| $recipients = explode(',', $this->getValue('recipient')); | ||
| sort($recipients); | ||
| return sha1(sprintf('schedule:%d,%s', $scheduleID, implode(',', $recipients)), true); |
| } | ||
| } | ||
|
|
||
| $db->commitTransaction(); | ||
| } | ||
|
|
||
| /** | ||
| * Get membership hash for the recipients in timeperiod entry for the given schedule |
There was a problem hiding this comment.
Please be more specific here and less verbose. You don't need to mention timeperiod entries here, that's obvious from the class already. Then, get imposes that the value may be be cached or so. At least, I don't see it fit with the implementation. It's not a getter. So maybe rename the method to calculate... or generate..., or even hash....
| ['id = ?' => $id] | ||
| ); | ||
| $prevTimeperiodEntriesCount -= 1; | ||
| } elseif ($prevTimeperiodEntriesCount === 1) { |
There was a problem hiding this comment.
This case doesn't update the entry at all
| $prevTimeperiodId = $this->getValue('timeperiod_id'); | ||
| $suppliedHash = $this->getValue('membership_hash'); | ||
| $calculatedHash = $this->getMembershipHashFromRecipients($scheduleId); | ||
| $changedMembers = ScheduleMember::on(Database::get()) |
There was a problem hiding this comment.
Here you're also actually only interested in the timeperiod.
| $db = Database::get(); | ||
| $db->beginTransaction(); | ||
| $binaryBehavior = new Binary([]); |
There was a problem hiding this comment.
Don't use the behavior directly here as well.
| ); | ||
|
|
||
| $prevTimeperiodEntriesCount -= 1; | ||
| if ($changedMembers->count() === 0) { |
There was a problem hiding this comment.
that check is redundant. If this case runs, the count is always zero.
|
As discussed. This is obsolete since it only complicates the code to achieve a so small benefit and is only relevant in a very specific edge-case. |
Currently a new time period gets created whenever a time period entry with the same group of contacts and/or contact groups for a given schedule. This nullifies the reason for existence of time periods.
Time periods should act as a bridge between the time period entries and the schedules.
Hence, time periods for a group of contacts and/or contact groups should be same across all time period entries for in a given schedule.
A new time period for a new time period entry should be generated only if there is no existing time period for the given schedule and the group of contacts and/contact groups. If another time period entry is created for the same group of contacts and/or contact groups in the same schedule, existing time period or these entries should be used.