Pairing: allow re-registering devices even if limit is reached #940
Conversation
Annopaolo
added
user experience
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #940 +/- ##
==========================================
+ Coverage 68.13% 68.17% +0.04%
==========================================
Files 281 281
Lines 7421 7435 +14
==========================================
+ Hits 5056 5069 +13
- Misses 2365 2366 +1 ☔ View full report in Codecov by Sentry. |
| @@ -223,7 +223,28 @@ defmodule Astarte.Pairing.Engine do | |||
| end | |||
| end | |||
|
|
|||
| defp verify_can_register_device(realm_name) do | |||
| defp verify_can_register_device(realm_name, device_id) do | |||
| # An already existsing device should always be able to retrieve a new credentials secret | |||
| @@ -117,6 +117,34 @@ defmodule Astarte.Pairing.Queries do | |||
| end | |||
| end | |||
|
|
|||
| def verify_already_registered_device(realm_name, device_id) do | |||
There was a problem hiding this comment.
In my opinion it would be more correct, since it returns {:ok, boolean}, to change the function to verify_already_registered_device?(...).
Also, the function is a bit too specific... might it be worth changing the pattern/logic to simply use select_device_by_id(realm_name, device_id) and manage externally whether it is present/registered in the db or not?
There was a problem hiding this comment.
On function name: we usually follow the check_* convention for {:ok, bool} results (see e.g. check_device_exists/2 in RealmManagement), good catch!
On function logic: code that is already in place uses the pattern of selecting only relevant information (see e.g. select_device_for_credentials_request/2 and select_device_for_info/2), I have followed the same pattern. Your suggestion would be interesting when refactoring Pairing!
Also, note that a function that performs the same query does already exist (check_already_registered_device/2), however it uses CQEx and therefore it needs a CQEx client rather than a simple realm name to run. I decided to keep engine.ex code simple and duplicate the functionality in queries.ex - not only for that, but also because we will soon™ remove CQEx entirely. Also, I had to change that check_yadayada function to verify_yadayada, as it does not return an {:ok, bool} but :ok/{:error/reason}
The device registration limit set for an Astarte realm is meant to prevent registration of new devices when the limit is reached. However, it blocked re-registration of already existing devices. Allow to re-register existing devices without checking if registration limit has been reached. See [astarte-platform#933](astarte-platform#933). Signed-off-by: Arnaldo Cesco <arnaldo.cesco@secomind.com>
Annopaolo
force-pushed
the
device-registration-upon-limit
branch
from
b369db6
to
394adc5
Compare
The device registration limit set for an Astarte realm is meant to prevent registration of new devices when the limit is reached. However, it blocked re-registration of already existing devices.
Allow to re-register existing devices without checking if registration limit has been reached.
Closes #933.