Skip to content
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

Sensorstoshow table column #1200

Merged
merged 25 commits into from
Oct 11, 2024
Merged

Sensorstoshow table column #1200

merged 25 commits into from
Oct 11, 2024

Conversation

joshuaunity
Copy link
Contributor

@joshuaunity joshuaunity commented Oct 2, 2024

Description

  • Added sensors_to_show column in generic_asset table
  • Copy over sensors_to_show data from inside the attributes column to the sensors_to_show column.
  • Logic to copy over data from inside the attributes field to the sensors_to_show field for new assets or newly added sensors

Look & Feel

There isn't any visual change.

Screenshot from 2024-10-02 08-03-42

How to test

  1. Run the new migrations with the command: flexmeasures db upgrade
  2. Visit the asset detail page: Asset Detail

Further Improvements

The function name process_sensors, there could be a better name for it, I am open to suggestions

Related Items

This PR closes the second task in #1076


  • 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

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>
@joshuaunity joshuaunity self-assigned this Oct 2, 2024
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>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The db migration works well, it seems!

I encountered a problem when visiting an asset page, though. Here is the db content (after migration):

 id |            name            | latitude | longitude |              attributes               | parent_asset_id | generic_asset_type_id | account_id | consumption_price_sensor_id | production_price_sensor_id | sensors_to_show  
----+----------------------------+----------+-----------+---------------------------------------+-----------------+-----------------------+------------+-----------------------------+----------------------------+------------------
 32 | Leaf van XY |   22 |    33 | {"sensors_to_show": [14, 73, 74, 75]} |                 |                     5 |          1 |                             |                            | [14, 73, 74, 75]

Looks okay to me. But the error (when visiting http://localhost:5000/assets/32) is:

  File "/home/nicolas/workspace/seita/flexmeasures/flexmeasures/ui/crud/assets/utils.py", line 111, in process_internal_api_response
    asset = GenericAsset(
  File "<string>", line 4, in __init__
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/state.py", line 571, in _initialize_instance
    with util.safe_reraise():
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/util/langhelpers.py", line 146, in __exit__
    raise exc_value.with_traceback(exc_tb)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/state.py", line 569, in _initialize_instance
    manager.original_init(*mixed[1:], **kwargs)
  File "/home/nicolas/workspace/seita/flexmeasures/flexmeasures/data/models/generic_assets.py", line 145, in __init__
    super().__init__(*args, **kwargs)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/decl_base.py", line 2170, in _declarative_constructor
    setattr(self, k, kwargs[k])
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 537, in __set__
    self.impl.set(
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 1277, in set
    value = self.fire_replace_event(
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 1292, in fire_replace_event
    value = fn(
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/events.py", line 2573, in wrap
    return fn(target, *arg)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/ext/mutable.py", line 536, in set_
    value = cls.coerce(key, value)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/ext/mutable.py", line 972, in coerce
    return Mutable.coerce(key, value)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/ext/mutable.py", line 455, in coerce
    raise ValueError(msg % (key, type(value)))
ValueError: Attribute 'sensors_to_show' does not accept objects of type <class 'str'>

Which extra value do we get from using MutableList, actually? Better protection against other values comin in?

documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/utils/coding_utils.py Outdated Show resolved Hide resolved
flexmeasures/utils/coding_utils.py Outdated Show resolved Hide resolved
flexmeasures/utils/coding_utils.py Outdated Show resolved Hide resolved
flexmeasures/utils/coding_utils.py Outdated Show resolved Hide resolved
flexmeasures/utils/coding_utils.py Outdated Show resolved Hide resolved
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
@joshuaunity
Copy link
Contributor Author

joshuaunity commented Oct 3, 2024

The db migration works well, it seems!

I encountered a problem when visiting an asset page, though. Here is the db content (after migration):

 id |            name            | latitude | longitude |              attributes               | parent_asset_id | generic_asset_type_id | account_id | consumption_price_sensor_id | production_price_sensor_id | sensors_to_show  
----+----------------------------+----------+-----------+---------------------------------------+-----------------+-----------------------+------------+-----------------------------+----------------------------+------------------
 32 | Leaf van XY |   22 |    33 | {"sensors_to_show": [14, 73, 74, 75]} |                 |                     5 |          1 |                             |                            | [14, 73, 74, 75]

Looks okay to me. But the error (when visiting http://localhost:5000/assets/32) is:

  File "/home/nicolas/workspace/seita/flexmeasures/flexmeasures/ui/crud/assets/utils.py", line 111, in process_internal_api_response
    asset = GenericAsset(
  File "<string>", line 4, in __init__
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/state.py", line 571, in _initialize_instance
    with util.safe_reraise():
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/util/langhelpers.py", line 146, in __exit__
    raise exc_value.with_traceback(exc_tb)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/state.py", line 569, in _initialize_instance
    manager.original_init(*mixed[1:], **kwargs)
  File "/home/nicolas/workspace/seita/flexmeasures/flexmeasures/data/models/generic_assets.py", line 145, in __init__
    super().__init__(*args, **kwargs)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/decl_base.py", line 2170, in _declarative_constructor
    setattr(self, k, kwargs[k])
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 537, in __set__
    self.impl.set(
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 1277, in set
    value = self.fire_replace_event(
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/attributes.py", line 1292, in fire_replace_event
    value = fn(
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/orm/events.py", line 2573, in wrap
    return fn(target, *arg)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/ext/mutable.py", line 536, in set_
    value = cls.coerce(key, value)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/ext/mutable.py", line 972, in coerce
    return Mutable.coerce(key, value)
  File "/home/nicolas/envs/fm-3.10/lib/python3.10/site-packages/sqlalchemy/ext/mutable.py", line 455, in coerce
    raise ValueError(msg % (key, type(value)))
ValueError: Attribute 'sensors_to_show' does not accept objects of type <class 'str'>

Which extra value do we get from using MutableList, actually? Better protection against other values comin in?

I came across this as well but I am yet to find where this action is occurring

I switched to using mutable list, I think cause the field holds a list but I think there was something else that made me use but cant quite remember

@nhoening
Copy link
Contributor

nhoening commented Oct 3, 2024

It seems that entries of the list need to be of a certain type, and that a string does not do. But I believe a simple string is also valid JSON?

You can also play around with this directly, opening flexmeasures shell, importting GenericAsset and try creating one in various ways. That might be most effective.

Or maybe we should try not using the MutableList, just making it a JSON field and see if that solves this issue?

…e <class 'str'> err

Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
@joshuaunity
Copy link
Contributor Author

does not accept objects of type <class 'str'>

I actually was able to find where the issue was, a section of the code was incorrectly parsing a string (JSON) instead of a list of JSON objects, which led to the error. After identifying this part, I made a small conversion to ensure it properly handles a list of JSON objects, and now it works as expected.

Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding where the problem was.
I found some improvements.

Also, can you add a form field to edit sensors_to_show in the asset form, so users are still able to edit in the UI?

documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/data/models/generic_assets.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets/views.py Outdated Show resolved Hide resolved
flexmeasures/data/models/generic_assets.py Outdated Show resolved Hide resolved
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>
@joshuaunity joshuaunity requested review from nhoening and removed request for Flix6x October 7, 2024 10:51
Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
joshuaunity and others added 3 commits October 9, 2024 10:38
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
* chore: in progress

Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>

* chore: added to changelog

Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>

---------

Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…d from db downgrade

Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the downgrade and it works well also! :)

@nhoening nhoening merged commit bffab47 into main Oct 11, 2024
8 of 9 checks passed
@nhoening nhoening deleted the sensorstoshow-table-solumn branch October 11, 2024 12:09
@Flix6x Flix6x added this to the 0.24.0 milestone Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants