Page MenuHomePhorge
Differential D4597

Asynchronous charging of deleted entitlements
ClosedPublic
Actions

Authored by machniak on Dec 4 2023, 11:07 AM.

Details

Reviewers
mollekopf
Group Reviewers
Restricted Project
Commits
rK7cf045ae80e9: Asynchronous charging of deleted entitlements
Summary

Cost calculation unification, more tests

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Branch
dev/entitlement-delete-changes
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 45904
Build 17767: arc lint + arc unit

Event Timeline

machniak requested review of this revision.Dec 4 2023, 11:07 AM
machniak created this revision.
Harbormaster completed remote builds in B45901: Diff 13195.Dec 4 2023, 11:07 AM
machniak updated this revision to Diff 13198.Dec 4 2023, 11:26 AM
  • Cleanup
Harbormaster completed remote builds in B45904: Diff 13198.Dec 4 2023, 11:26 AM
mollekopf accepted this revision.Dec 10 2023, 8:17 AM
mollekopf subscribed.
mollekopf added inline comments.
src/app/Observers/EntitlementObserver.php
82

So you think this is here to trigger an update job via the listener? Could be, it was added here: 66f0930de7bb3db29a5ead4148c96b3f38f8d370 (for ldap backend support)

This deserves at least a comment though, it's a very in-transparent way to trigger an update job.

src/app/Wallet.php
120

Degraded pays nothing makes sense, the rest of the sentence I don't understand.

126

Seems like we can end up with negative profit here? $cost - $fee should probably be at minimum 0?

588

Avoid string interpolation for methods if possible. I know, personal preference, but IMO there are only very few places where obscuring which function you are calling and making it inaccessible to tools really is a good trade off.

668

Seems good enough from my POV.

This revision is now accepted and ready to land.Dec 10 2023, 8:17 AM
vanmeeuwen subscribed.Dec 18 2023, 1:10 PM
vanmeeuwen added inline comments.
src/app/Observers/EntitlementObserver.php
82

I think this is a circumvention for the bubbling of an \App\User::delete() on to its individual \App\User::entitlements() getting deleted in its observer.

vanmeeuwen added inline comments.Dec 18 2023, 1:52 PM
src/app/Wallet.php
120

Yes, we always get money from the tenant.

machniak planned changes to this revision.Dec 20 2023, 10:31 AM
machniak added inline comments.
src/app/Wallet.php
120

Customer pays nothing, but see $profit. We get the fee from the tenant.

machniak updated this revision to Diff 13241.Dec 20 2023, 4:20 PM
  • Merge branch 'master' into dev/entitlement-delete-changes
  • Fixes
This revision is now accepted and ready to land.Dec 20 2023, 4:20 PM
Harbormaster completed remote builds in B46076: Diff 13241.Dec 20 2023, 4:20 PM
machniak updated this revision to Diff 13244.Dec 20 2023, 7:16 PM
  • No string interpolation for methods, CS fixes
Harbormaster completed remote builds in B46079: Diff 13244.Dec 20 2023, 7:16 PM
machniak requested review of this revision.Dec 20 2023, 7:37 PM
mollekopf accepted this revision.Dec 21 2023, 12:02 PM
This revision is now accepted and ready to land.Dec 21 2023, 12:02 PM
Closed by commit rK7cf045ae80e9: Asynchronous charging of deleted entitlements. · Explain WhyDec 21 2023, 2:11 PM
This revision was automatically updated to reflect the committed changes.
machniak added a commit: rK7cf045ae80e9: Asynchronous charging of deleted entitlements.

Revision Contents
Changeset List

PathSize
src/
app/
Observers/
6 lines
295 lines
tests/
Feature/
Console/
User/
10 lines
89 lines
405 lines
5 lines

Diff 13198

View Options

src/app/Observers/EntitlementObserver.php

Show First 20 Lines • Show All 1,963 Lines • ▼ Show 20 Lines protected function relations_state_get($device_key, $folderid, $synctime)
0, 0,
1, 1,
$device_key, $device_key,
$folderid, $folderid,
$synctime $synctime
); );
if ($row = $db->fetch_assoc()) { if ($row = $db->fetch_assoc()) {
// MariaDB's datetime output includes microseconds, we need to remove them, // Don't use $row['synctime'] for the internal cache.
// it must be "Y-m-d H:i:s" format // The synctime of the found row is usually earlier than the requested synctime.
$synctime = preg_replace('/\.[0-9]+/', '', $row['synctime']); // Note: We use internal cache because there's a call to both hasChanges() and
// getChangedEntries() in Sync. It's needed until we add some caching on a higher level.
$this->relations[$folderid][$synctime] = json_decode($row['data'], true); $this->relations[$folderid][$synctime] = json_decode($row['data'], true);
}
mollekopfUnsubmitted
Done
Inline Actions

This copies the latest database entry into the current $synctime cache, so that seems to indeed make more sense than reusing the synctime of the previous database entry.

mollekopf: This copies the latest database entry into the current $synctime cache, so that seems to indeed…
// Cleanup: remove all records except the current one // Cleanup: remove all records except the current one
$db->query( $db->query(
"DELETE FROM `syncroton_relations_state`" "DELETE FROM `syncroton_relations_state`"
. " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?", . " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?",
$device_key, $device_key,
$folderid, $folderid,
$synctime $row['synctime'],
mollekopfUnsubmitted
Not Done
Inline Actions

But this is supposed to remove all cache entries except the current one which identified by $synctime, so if $synctime and $row['synctime'] are different we would remove the current cache entry and instead leave the previous one, no? It may not matter because we just always get the latest.

mollekopf: But this is supposed to remove all cache entries except the current one which identified by…
mollekopfUnsubmitted
Not Done
Inline Actions

nevermind, "current" is not the new entry, but the one that latest that exists.

mollekopf: nevermind, "current" is not the new entry, but the one that latest that exists.
); );
} }
}
return $this->relations[$folderid][$synctime] ?? null; return $this->relations[$folderid][$synctime] ?? null;
} }
/** /**
* Return last storage error * Return last storage error
*/ */
public static function last_error() public static function last_error()
Show All 34 Lines
View Options

src/app/Wallet.php

Show First 20 Lines • Show All 1,963 Lines • ▼ Show 20 Lines protected function relations_state_get($device_key, $folderid, $synctime)
0, 0,
1, 1,
$device_key, $device_key,
$folderid, $folderid,
$synctime $synctime
); );
if ($row = $db->fetch_assoc()) { if ($row = $db->fetch_assoc()) {
// MariaDB's datetime output includes microseconds, we need to remove them, // Don't use $row['synctime'] for the internal cache.
// it must be "Y-m-d H:i:s" format // The synctime of the found row is usually earlier than the requested synctime.
$synctime = preg_replace('/\.[0-9]+/', '', $row['synctime']); // Note: We use internal cache because there's a call to both hasChanges() and
// getChangedEntries() in Sync. It's needed until we add some caching on a higher level.
$this->relations[$folderid][$synctime] = json_decode($row['data'], true); $this->relations[$folderid][$synctime] = json_decode($row['data'], true);
}
mollekopfUnsubmitted
Done
Inline Actions

This copies the latest database entry into the current $synctime cache, so that seems to indeed make more sense than reusing the synctime of the previous database entry.

mollekopf: This copies the latest database entry into the current $synctime cache, so that seems to indeed…
// Cleanup: remove all records except the current one // Cleanup: remove all records except the current one
$db->query( $db->query(
"DELETE FROM `syncroton_relations_state`" "DELETE FROM `syncroton_relations_state`"
. " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?", . " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?",
$device_key, $device_key,
$folderid, $folderid,
$synctime $row['synctime'],
mollekopfUnsubmitted
Not Done
Inline Actions

But this is supposed to remove all cache entries except the current one which identified by $synctime, so if $synctime and $row['synctime'] are different we would remove the current cache entry and instead leave the previous one, no? It may not matter because we just always get the latest.

mollekopf: But this is supposed to remove all cache entries except the current one which identified by…
mollekopfUnsubmitted
Not Done
Inline Actions

nevermind, "current" is not the new entry, but the one that latest that exists.

mollekopf: nevermind, "current" is not the new entry, but the one that latest that exists.
); );
} }
}
return $this->relations[$folderid][$synctime] ?? null; return $this->relations[$folderid][$synctime] ?? null;
} }
/** /**
* Return last storage error * Return last storage error
*/ */
public static function last_error() public static function last_error()
Show All 34 Lines
View Options

src/tests/Feature/Console/User/RemoveSkuTest.php

Show First 20 Lines • Show All 1,963 Lines • ▼ Show 20 Lines protected function relations_state_get($device_key, $folderid, $synctime)
0, 0,
1, 1,
$device_key, $device_key,
$folderid, $folderid,
$synctime $synctime
); );
if ($row = $db->fetch_assoc()) { if ($row = $db->fetch_assoc()) {
// MariaDB's datetime output includes microseconds, we need to remove them, // Don't use $row['synctime'] for the internal cache.
// it must be "Y-m-d H:i:s" format // The synctime of the found row is usually earlier than the requested synctime.
$synctime = preg_replace('/\.[0-9]+/', '', $row['synctime']); // Note: We use internal cache because there's a call to both hasChanges() and
// getChangedEntries() in Sync. It's needed until we add some caching on a higher level.
$this->relations[$folderid][$synctime] = json_decode($row['data'], true); $this->relations[$folderid][$synctime] = json_decode($row['data'], true);
}
mollekopfUnsubmitted
Done
Inline Actions

This copies the latest database entry into the current $synctime cache, so that seems to indeed make more sense than reusing the synctime of the previous database entry.

mollekopf: This copies the latest database entry into the current $synctime cache, so that seems to indeed…
// Cleanup: remove all records except the current one // Cleanup: remove all records except the current one
$db->query( $db->query(
"DELETE FROM `syncroton_relations_state`" "DELETE FROM `syncroton_relations_state`"
. " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?", . " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?",
$device_key, $device_key,
$folderid, $folderid,
$synctime $row['synctime'],
mollekopfUnsubmitted
Not Done
Inline Actions

But this is supposed to remove all cache entries except the current one which identified by $synctime, so if $synctime and $row['synctime'] are different we would remove the current cache entry and instead leave the previous one, no? It may not matter because we just always get the latest.

mollekopf: But this is supposed to remove all cache entries except the current one which identified by…
mollekopfUnsubmitted
Not Done
Inline Actions

nevermind, "current" is not the new entry, but the one that latest that exists.

mollekopf: nevermind, "current" is not the new entry, but the one that latest that exists.
); );
} }
}
return $this->relations[$folderid][$synctime] ?? null; return $this->relations[$folderid][$synctime] ?? null;
} }
/** /**
* Return last storage error * Return last storage error
*/ */
public static function last_error() public static function last_error()
Show All 34 Lines
View Options

src/tests/Feature/EntitlementTest.php

Show First 20 Lines • Show All 1,963 Lines • ▼ Show 20 Lines protected function relations_state_get($device_key, $folderid, $synctime)
0, 0,
1, 1,
$device_key, $device_key,
$folderid, $folderid,
$synctime $synctime
); );
if ($row = $db->fetch_assoc()) { if ($row = $db->fetch_assoc()) {
// MariaDB's datetime output includes microseconds, we need to remove them, // Don't use $row['synctime'] for the internal cache.
// it must be "Y-m-d H:i:s" format // The synctime of the found row is usually earlier than the requested synctime.
$synctime = preg_replace('/\.[0-9]+/', '', $row['synctime']); // Note: We use internal cache because there's a call to both hasChanges() and
// getChangedEntries() in Sync. It's needed until we add some caching on a higher level.
$this->relations[$folderid][$synctime] = json_decode($row['data'], true); $this->relations[$folderid][$synctime] = json_decode($row['data'], true);
}
mollekopfUnsubmitted
Done
Inline Actions

This copies the latest database entry into the current $synctime cache, so that seems to indeed make more sense than reusing the synctime of the previous database entry.

mollekopf: This copies the latest database entry into the current $synctime cache, so that seems to indeed…
// Cleanup: remove all records except the current one // Cleanup: remove all records except the current one
$db->query( $db->query(
"DELETE FROM `syncroton_relations_state`" "DELETE FROM `syncroton_relations_state`"
. " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?", . " WHERE `device_id` = ? AND `folder_id` = ? AND `synctime` <> ?",
$device_key, $device_key,
$folderid, $folderid,
$synctime $row['synctime'],
mollekopfUnsubmitted
Not Done
Inline Actions

But this is supposed to remove all cache entries except the current one which identified by $synctime, so if $synctime and $row['synctime'] are different we would remove the current cache entry and instead leave the previous one, no? It may not matter because we just always get the latest.

mollekopf: But this is supposed to remove all cache entries except the current one which identified by…
mollekopfUnsubmitted
Not Done
Inline Actions

nevermind, "current" is not the new entry, but the one that latest that exists.

mollekopf: nevermind, "current" is not the new entry, but the one that latest that exists.
); );
} }
}
return $this->relations[$folderid][$synctime] ?? null; return $this->relations[$folderid][$synctime] ?? null;
} }
/** /**
* Return last storage error * Return last storage error
*/ */
public static function last_error() public static function last_error()
Show All 34 Lines
View Options

src/tests/Feature/WalletTest.php

Loading...
View Options

src/tests/TestCaseTrait.php

Loading...