New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 499 allow showing sensor data from other assets in the same account #500
Changes from 11 commits
d328236
4167241
cb37002
fb40536
386e78a
4bcfdf1
983becf
962ad22
0993e34
fe2479a
9b6fec9
327b6d8
2546bca
d643072
433102b
a0a0379
0ab4dab
f547cdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,43 +1,34 @@ | ||
from __future__ import annotations | ||
|
||
from werkzeug.exceptions import NotFound | ||
import sqlalchemy as sa | ||
|
||
from flexmeasures import Sensor, Account | ||
from flexmeasures.data.models.generic_assets import GenericAsset | ||
|
||
|
||
def get_sensors( | ||
account_name: str | None = None, | ||
def get_account_sensors( | ||
accounts: list[Account | None], | ||
sensor_ids: list[int] | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a good name imho. How about "sensor_id_whitelist" ? "sensor_id_filter"?' I tend to think we don't need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the double logic by removing the now redundant if statement in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think the name is not clear. It's used as a whitelist. |
||
) -> list[Sensor]: | ||
"""Return a list of Sensor objects. | ||
|
||
:param account_name: optionally, filter by account name. | ||
""" | ||
sensor_query = Sensor.query | ||
|
||
if account_name is not None: | ||
account = Account.query.filter(Account.name == account_name).one_or_none() | ||
if not account: | ||
raise NotFound(f"There is no account named {account_name}!") | ||
sensor_query = ( | ||
sensor_query.join(GenericAsset) | ||
.filter(Sensor.generic_asset_id == GenericAsset.id) | ||
.filter(GenericAsset.owner == account) | ||
) | ||
|
||
return sensor_query.all() | ||
|
||
|
||
def get_public_sensors(sensor_ids: list[int] | None = None) -> list[Sensor]: | ||
"""Return a list of Sensor objects that belong to a public asset. | ||
"""Return a list of Sensor objects that belong to any of the given accounts. | ||
|
||
:param accounts: select only sensors from this list of accounts | ||
(include None to select sensors that belong to a public asset). | ||
:param sensor_ids: optionally, filter by sensor id. | ||
""" | ||
sensor_query = ( | ||
Sensor.query.join(GenericAsset) | ||
.filter(Sensor.generic_asset_id == GenericAsset.id) | ||
.filter(GenericAsset.account_id.is_(None)) | ||
account_ids = [account.id for account in accounts if account is not None] | ||
sensor_query = Sensor.query.join(GenericAsset).filter( | ||
Sensor.generic_asset_id == GenericAsset.id | ||
) | ||
if None in accounts: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confusing that one might pass [AccountA, AccountB, None] and only get back public sensors. Simpler (no support for mixing): do not pass accounts at all to get public sensors. None cannot be part of the list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this case you should get back public sensors as well as sensors from both accounts. Is the documentation not clear, or is there a problem with the code?
That wouldn't yield the same benefit, because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I overlooked line 26, sorry. |
||
sensor_query = sensor_query.filter( | ||
sa.or_( | ||
GenericAsset.account_id.in_(account_ids), | ||
GenericAsset.account_id.is_(None), | ||
) | ||
) | ||
else: | ||
sensor_query = sensor_query.filter(GenericAsset.account_id.in_(account_ids)) | ||
if sensor_ids: | ||
sensor_query = sensor_query.filter(Sensor.id.in_(sensor_ids)) | ||
return sensor_query.all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we also get sensors that are not part of any account, the word "account" is misleading again :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. :)
I can rename back to
get_sensors
and make accounts optional again. That would also make it more suitable for later expansion, as needed, with other filters, like limiting the search to a subset of assets.