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
Issue 499 allow showing sensor data from other assets in the same account #500
Conversation
…count Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Pull Request Test Coverage Report for Build 3219270038
💛 - Coveralls |
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.
Great. Just one small signature simplification.
@@ -7,14 +7,25 @@ | |||
|
|||
|
|||
def get_sensors( | |||
account_id: int | None = None, |
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.
I believe we should keep this to one argument.
If it's int: query for account ID.
If it's string: query for name.
Otherwise, it's an error.
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.
Thanks. Your idea also gave me the idea to avoid some duplicate queries to the Account table, and to filter the query of the sensors table.
…d or name), and avoid duplicate queries of a known account Signed-off-by: F.N. Claessen <felix@seita.nl>
…ame owner Signed-off-by: F.N. Claessen <felix@seita.nl>
… from the same account, which may be a lot) Signed-off-by: F.N. Claessen <felix@seita.nl>
…ng_sensor_data_from_other_assets_in_the_same_account # Conflicts: # documentation/changelog.rst
@@ -7,23 +7,32 @@ | |||
|
|||
|
|||
def get_sensors( | |||
account_name: str | None = None, | |||
account: Account | int | str | None = None, | |||
sensor_ids: list[int] | None = None, |
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.
Not a good name imho. How about "sensor_id_whitelist" ? "sensor_id_filter"?'
I tend to think we don't need it.
We now have double logic as we already do if sensor.id in sensor_ids
in sensors_to_show()
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.
I removed the double logic by removing the now redundant if statement in sensors_to_show()
.
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.
I still think the name is not clear. It's used as a whitelist.
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…t, or None for public sensors (which makes get_public_sensors obsolete) Signed-off-by: F.N. Claessen <felix@seita.nl>
Sorry to ask you to review once more. Your suggested name change led me to repurpose |
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.
Getting closer :D
) | ||
if None in accounts: |
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.
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 comment
The 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.
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?
Simpler (no support for mixing): do not pass accounts at all to get public sensors. None cannot be part of the list.
That wouldn't yield the same benefit, because sensors_to_show()
would need to call the function twice, resulting in two queries instead of one.
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.
I overlooked line 26, sorry.
|
||
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( |
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.
@@ -7,23 +7,32 @@ | |||
|
|||
|
|||
def get_sensors( | |||
account_name: str | None = None, | |||
account: Account | int | str | None = None, | |||
sensor_ids: list[int] | None = None, |
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.
I still think the name is not clear. It's used as a whitelist.
…gain, and rename filter argument Signed-off-by: F.N. Claessen <felix@seita.nl>
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.
I approve with two small conditions (sorry)
:param account_name: optionally, filter by account name. | ||
:param accounts: optionally, select only sensors from this list of accounts | ||
(include None to select sensors that belong to a public asset). | ||
:param filter_by_sensor_ids: optionally, filter by sensor id. |
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.
"optionally, filter out any sensors which are not in this whitelist"
|
||
:param account_name: optionally, filter by account name. | ||
:param accounts: optionally, select only sensors from this list of accounts | ||
(include None to select sensors that belong to a public asset). |
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.
I'm not a fan of the None argument to indicate public assets.
I believe an explicit parameter "include_public_assets", which defaults to False, would make it easier to see what's going on.
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
This PR lets the asset page also show sensor data from other assets that belong to the same account.