feat: Adapting flex-config to SensorsToShowSchema#1904
feat: Adapting flex-config to SensorsToShowSchema#1904joshuaunity wants to merge 37 commits intomainfrom
Conversation
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Documentation build overview
Show files changed (9 files in total): 📝 9 modified | ➕ 0 added | ➖ 0 deleted
|
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Co-authored-by: Felix Claessen <30658763+Flix6x@users.noreply.github.com> Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
nhoening
left a comment
There was a problem hiding this comment.
Alright - I could get something to work here, which is nice! There are some bigger TODOs, but the approach is valid!
I set this JSON:
[{"title": "P3", "sensors": [44208]}, {"plots": [{"asset": 3749, "flex-model": "soc-min"}], "title": "flex-model-tests"}]
I believe it is very close to the example you give in the PR description.
Note: my method of testing was to set the attribute via SwaggerUI.
That meant I had to escape quotes in my JSON - do you have a nicer workaround of setting these values?
I have some documentation remarks and others inline.
Here are my larger remarks. Maybe you deliberately do not want to work on anything in the graph modal dialogue - then take point 2 and 4 as potential inspiration for the PR where you actually work on that :)
Test
I noted that inline, too. Tests for the schema should include at least one of the newer input values.
Sensor description
In the UI, I then see for this graph:
ID: [undefined](http://localhost:5000/sensors/undefined), Unit: undefined, Name: undefined, Asset: undefined, Account: PUBLIC
Even when the sensor's data is loaded okay.
- We need to display here which flex-config field was used (e.g. "flex_model.soc_min of asset 3749")
- And then the sensor information as we do it for others.
Fixed value on flex config
For the fixed soc_mon ("30kWh"), I see no data
After I took a better look at the code, it seems there is no support yet for fixed values from the flex config - maybe a misunderstanding from an earlier question.
When fixed values are in the flex config, I do want them supported here.
Not (yet) defined flex config fields
It didn't matter if the soc-min field had been defined on asset 3749's flex model or not. I saw the same info about my plot in the left part of the modal dialogue.
If the field does not exist on the asset's flex-model, we should probably say so in the UI. The user should see the plot, as the intention to show data gotten via the flex config, but that the field still needs to be set.
Validate incoming data
I noticed that the incoming sensors_to_show JSON (via the API) is being checked on well-formed JSON, but not against the SensorsToShowSchema. See here, where that could actually happen.
That might make a good addition. I know your modal dialogue probably only sends valid content, but for me testing via the API and also for debugging and seeing bugs earlier, it is a good idea to only let good data in. I hope it would be rather easy to do?
Co-authored-by: Nicolas Höning <nicolas@seita.nl> Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
Co-authored-by: Nicolas Höning <nicolas@seita.nl> Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
Co-authored-by: Nicolas Höning <nicolas@seita.nl> Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
Co-authored-by: Nicolas Höning <nicolas@seita.nl> Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
|
Note: @joshuaunity and I discussed that my third note (not only support sensors, but also fixed flex config values like "30kW") will be split out into a follow-up PR after this one and the UI dialogue refactoring are done. However, I would then wait with merging this until v0.31 is out, so we can release all three PRs together for v0.32 |
nhoening
left a comment
There was a problem hiding this comment.
Minor doc comments, mostly seems good.
But this test is now failing (not raising a ValidationError):
def test_dict_misspelled_title_key():
schema = SensorsToShowSchema()
input_value = [{"titel": "Temperature", "sensor": 42}] # Misspelled 'title'
with pytest.raises(ValidationError, match="Dictionary must contain a 'title' key."):
schema.deserialize(input_value)
Now we don't require the title attribute anymore - but we should maybe fail if we get unexpected keys in a graph dict.
That would help the user and us in debugging.
So we have a list of fields we expect ("plots", "sensors", "sensor", "title", ... - any others?) and can raise a validation error if we get any other.
If an invalid key was added, it doesn't break the UI in any way, so this validation would just be a nice-to-have. Are there scenarios you think foreign keys would cause a problem? |
…changes Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
…sures/flexmeasures into feat/allow-Ssensorstoshow-schema
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
My motivation was that it's quicker for a bug to be spotted, e.g. is the frontend somehow sends an unknown field. But okay let's move on. |
nhoening
left a comment
There was a problem hiding this comment.
Thanks!
Please confirm testing the first tutorial is still okay.
@joshuaunity have you confirmed this? |
I'll circle back on this after the completion of the UI work |
Description
This PR adopts a new shape for our SensorsToShowSchema, and also includes the adaptation of the flex-config into the schema as well.
TODO
Look & Feel
None
How to test
This new feature can only be interacted with through the API for now.
http://localhost:5000/api/v3_0/assets/<asset_id>sensor_to_showwith the below dataNOTE: for the below, use sensors that are under the current asset being updated
Further Improvements
Support for the addition of fixed values as plot entries.
Related Items
This PR closes #1880
Sign-off