Search sensors in API by asset

[joshuaunity]

Description

Search sensors by asset ID

Look & Feel

How to test

Apply the asset_id query param to the sensors API http://localhost:5000/api/v3_0/sensors?page=1&per_page=10&asset_id=2

Simply put in the ID of any valid asset

Further Improvements

None

Related Items

This PR closes: #1209

---

• I agree to contribute to the project under Apache 2 License.
• To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

[nhoening]

Thanks.

Two things I want to see besides what the comments say:

• Please add a test for the new parameter
• The filtering now checks Sensor.name and Account.name. See line 207f
  We should also add GenericAsset.name.ilike(f"%{term}%")

[nhoening]

if all_accessible:
    consultancy_account_ids: list = [
        acc.consultancy_account_id for acc in accounts
    ]
    account_ids.extend(consultancy_account_ids)

1. This seems now to be the only use of accounts (we use account_ids in the query).
   I guess we don't need that variable, really.
2. What do we want to do here? We want to add accounts which the current user can see, in addition to his own account (or the one they sent in as parameter).

• if their own account is not in the list, add it here
• more importantly, the code right now adds the current users' consultancy account, for which they have no rights. This should add the accounts which have the current users' account as consultancy account - and that might require a query, like above for the child assets.

[joshuaunity]

if all_accessible:
    consultancy_account_ids: list = [
        acc.consultancy_account_id for acc in accounts
    ]
    account_ids.extend(consultancy_account_ids)

1. This seems now to be the only use of accounts (we use account_ids in the query).
   I guess we don't need that variable, really.
2. What do we want to do here? We want to add accounts which the current user can see, in addition to his own account (or the one they sent in as parameter).

• if their own account is not in the list, add it here
• more importantly, the code right now adds the current users' consultancy account, for which they have no rights. This should add the accounts which have the current users' account as consultancy account - and that might require a query, like above for the child assets.

Oh I see, I think it makes sense to remove the accounts variable. If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

[nhoening]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

[joshuaunity]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

Ok. For the second part you mean something like this right

consultancy_accounts = (
                db.session.query(Account)
                .filter(Account.consultancy_account_id == account.id)
                .all()
            )
            consultancy_account_ids: list = [acc.id for acc in consultancy_accounts]

[joshuaunity]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

Regarding the first part, on a closer look, it seems unlikely that the current user’s account would be missing from the list, given that we’re initializing account_ids with [account.id], which includes the current user's account ID by default. what do you think?

[nhoening]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

Regarding the first part, on a closer look, it seems unlikely that the current user’s account would be missing from the list, given that we’re initializing account_ids with [account.id], which includes the current user's account ID by default. what do you think?

Only by default, they can send us another, I believe.

[nhoening]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

Ok. For the second part you mean something like this right

consultancy_accounts = (
                db.session.query(Account)
                .filter(Account.consultancy_account_id == account.id)
                .all()
            )
            consultancy_account_ids: list = [acc.id for acc in consultancy_accounts]

Seems about right. We should also check if the current user has the role consultant, which makes sure he would be allowed to read data from these accounts. If not, we don't add consultancy accounts to the list.

[joshuaunity]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

Regarding the first part, on a closer look, it seems unlikely that the current user’s account would be missing from the list, given that we’re initializing account_ids with [account.id], which includes the current user's account ID by default. what do you think?

Only by default, they can send us another, I believe.

Oh yeah that's true, for a second I forgot that the user can specify a different account

[joshuaunity]

If I understand you correctly, the approach you said here means that I do a conditional statement to check if the users account is currently in the account_ids list and if not I add it my self, right?

Yes, but the part after that is even more important to fix.

Ok. For the second part you mean something like this right

consultancy_accounts = (
                db.session.query(Account)
                .filter(Account.consultancy_account_id == account.id)
                .all()
            )
            consultancy_account_ids: list = [acc.id for acc in consultancy_accounts]

Seems about right. We should also check if the current user has the role consultant, which makes sure he would be allowed to read data from these accounts. If not, we don't add consultancy accounts to the list.

ok, I'll work on that. This will mean that the all_accessible parameter is majorly for adding consultant accounts, cause looking at the code that's all the parameter is currently doing

[joshuaunity]

ok, I'll work on that. This will mean that the all_accessible parameter is majorly for adding consultant accounts, cause looking at the code that's all the parameter is currently doing

Have you seen this @nhoening

[nhoening]

This will mean that the all_accessible parameter is majorly for adding consultant accounts, cause looking at the code that's all the parameter is currently doing

Good observation.

I guess we have admins who can in principle search everywhere, but I'm not sure that has a valid use case here.
It would be straightforward to rename the parameter include_consultancy_clients so we model what happens now. And even for that I'm not sure what the use case would be.

[joshuaunity]

This will mean that the all_accessible parameter is majorly for adding consultant accounts, cause looking at the code that's all the parameter is currently doing

Good observation.

I guess we have admins who can in principle search everywhere, but I'm not sure that has a valid use case here. It would be straightforward to rename the parameter include_consultancy_clients so we model what happens now. And even for that I'm not sure what the use case would be.

I agree, it makes sense to rename it for now to clarify its purpose. We could also update line 184 from:

if include_public_assets or all_accessible:

to simply

if include_public_assets:

This would make the parameter’s function clearer and more straightforward.

What do you think?

[nhoening]

Yes that would actually be necessary

[joshuaunity]

Yes that would actually be necessary

ok then, let me work on that

[joshuaunity]

Yes that would actually be necessary

You can check it now, I have implemented the new changes.

[nhoening]

Looks pretty good, I just found one design issue left :(

[nhoening]

The docstring needs some updating to reflect we filter either by account or by asset.
I believe line 110 (and the corresponding code) could go.

And in the docs (and maybe in the code) we should say that include_consultancy_clients only applies if the user searches by account. (also if they gave neither, of course, then it is their own account).

On the other hand, include_public_assets works on both cases I think.

[nhoening]

Almost there for sure now:)

Can you add a test where we ask for account and not for an asset?

For future reference, check your test coverage here

[nhoening]

Thanks!

[joshuaunity]

Hi, @nhoening can this be merged today so I can do some testing on the edit form PR

[nhoening]

Sure I was waiting for tests to finish