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

Simplify addressing assets #37

Closed
nhoening opened this issue Feb 19, 2021 · 3 comments
Closed

Simplify addressing assets #37

nhoening opened this issue Feb 19, 2021 · 3 comments

Comments

@nhoening
Copy link
Contributor

Right now we address assets a lot by their name (possibly because that is more user-friendly). This happens in our data model (aggregation class Resource is initialised with the name of an asset or an asset group), the UI (session variable "resource") and maybe the API (validator assets_required, which also works for weather sensors and markets, expects asset information encoded in the entity addresses, and that the ID for assets but the name for markets and sensors, so it's unclear what effort is required).

The problems with this are that:

  • the asset name might not be url-safe (see Bug: broken link on asset page for some assets #19)
  • we have both display name and asset name, which can be set independently, and that can lead to confusing states (they can be quite different which would not be good and also the name needs to be unique if it is being used to identify assets, so we need good logic to ensure that).

In #20 I already attempted to make sure the display name is derived from the asset name (as I believed the asset name is the crucial functional field, the display name is for showing) but stopped when I realised how much code I would have to change.

We need a new policy here. Options:

  1. Assets (and sensors and markets) will only have an ID and a name. The name can be freely chosen but is just for display. Simplifies the model, but requires UI work (always longer than you think), breaking API changes and the solution for aggregated asset levels is not clear.
  2. Keep using asset.name but make it strictly dependent on display name (parameterised, so url-friendly), i.e. no user can edit it. Maybe rename it to ident_name or so.

In all cases, assets, sensors and markets should be modelled and treated in the same way (right now, markets and sensors have no id field for instance, but assets do).

@Flix6x
Copy link
Contributor

Flix6x commented Feb 27, 2021

Small correction: since Assets, Market and WeatherSensors subclass timely_beliefs.SensorDBMixin they all have an id field.

@Flix6x
Copy link
Contributor

Flix6x commented Nov 15, 2022

Can we mark this as resolved? Relevant database models and API versions are deprecated. All GenericAsset models have an ID now, and API version 3 uses only IDs to identify database models. Dashboard grouping of assets (by owner or GenericAssetType) also doesn't rely on the asset name anymore.

Is there anything left I'm missing?

@nhoening
Copy link
Contributor Author

Let's close. When we throw out older code, we'll see better if we forgot anything .

@Flix6x Flix6x closed this as completed Nov 15, 2022
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

No branches or pull requests

2 participants