Stop url encoding in UI templates

[Flix6x]

(See corresponding issue.)

[nhoening]

The other place where the encoding happened is in the Python view? Or did the browser do the encoding?

[Flix6x]

It was solely an empirical observation, but I would assume the browser always does it. The asset name is set as a url parameter by the assets page, which is then picked up by the analytics page to set the resource parameter in the session. That means the non-encoded name is expected (space rather than %20). What you end up seeing as the url in any browser (that I know of) is always the encoded url (%20 rather than space).

…

On Tue, Feb 9, 2021, 15:47 Nicolas Höning ***@***.***> wrote: ***@***.**** commented on this pull request. The other place where the encoding happened is in the Python view? Or did the browser do the encoding? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#20 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHJ5BS45Q3BKN4IBDPP3CVLS6FDI7ANCNFSM4XLDUZNA> .

[Flix6x]

After some discussion we decided to pass the newly chosen display name (from the asset edit form) through a function we already use to nicely parameterize resource names for use in both python and javascript:

from flexmeasures.utils.flexmeasures_inflection import parameterize

asset.name = parameterize(asset.display_name)

Nicolas knows best where exactly in our asset crud logic this should go.

Furthermore, we can lose the urlencode statements (this PR currently does exactly that), but only if we're sure that they aren't needed by various browsers. I checked Firefox and Chromium.

One more note: I am still assuming that the parameterized name would not change upon urlencoding. We could decide to make sure of that by triggering a urlencode on the result of our parameterize function (not on its input). What do you think?

[nhoening]

@Flix6x I decided not to change Asset.name based on Asset.display_name for now (on editing). I understand we only let users change the display_name in our own UI, but now I was looking at it from the API perspective, where you could be changing either.
If we want to make a change, we need to consistently base one on the other (in the data model, like removing the name field and making it a property which is dynamically based on display_name) or stop allowing them to be different. Anything else will be confusingly inconsistent (at least for [third-party] developers).

With regard to your other question, I tried this:

>>> from urllib.parse import quote  # this is what jinjia2 uses under the hood when we filter through 'urlencode'
>>> from flexmeasures.utils.flexmeasures_inflection import humanize, pluralize, parameterize
>>> s = "My* Asset!"
>>> quote(s)
'My%2A%20Asset%21'
>>> quote(parameterize(s))
'my_asset'

Feel free to test more.

[nhoening]

If we parameterise an Asset's name on initialisation, we get a situation where you pass a name to the new Asset, but then cannot use it to look for it later. This is why currently some tests are failing, I'd still have to fix more.

We can accept that for now, but it might be confusing.

All of this seems to add a lot of trouble. Why did we not just opt for an integer ID and a string name (and require that the name be unique)?

[nhoening]

@Flix6x Why does our parameterisation insist on making names which can be used "as a python or javascript variable name"? If we don't use that, fixing tests is somewhat easier. Do we need it?

[Flix6x]

I understand the difficulties here, and I fear this PR is now turning into an entirely different issue, namely: deprecating the use of asset.name as if it's an id. Which imo should be a separate issue. For me, this PR should only consist of the removal of the urlencode statements, resolving Issue #19.

Some clarifications from my side:

About deriving names from display names upon editing

You said you "decided not to change Asset.name based on Asset.display_name for now (on editing)", but the code that was already doing exactly that remains unaltered in flexmeasures/ui/crud/assets.py:

data["name"] = data["display_name"]  # both are part of the asset model

I also see you added a comment in flexmeasures/api/v2_0/implementations/assets.py:

    display_name = fields.Str(
        validate=validate.Length(min=4)
    )  # not required: can be derived from name

which suggests you want to derive the display name from the name (i.e. the other way around). I also saw you thought about humanizing the display name, and I'm glad you took that out again. Maybe this comment about deriving the display name from the name was connected to that line of thinking?

Making parameterized names invariable under urlencoding

Thanks for the code example. What I had in mind was that

quote(parameterize(s)) == parameterize(s)

is True for any string.

Why our parameterisation insist on making names which can be used "as a python or javascript variable name"

I had introduced the parameterization for resources (not individual assets), because the resource names were used to set up javascript variables for our dashboard map (containing layered groups of assets that can be toggled on or off), which in turn were set up by Jinja. Consider the next line in dashboard.html:

var {{ asset_groups[asset_group].parameterized_name }}_icon = new L.DivIcon({

The parameterized_name is first parsed by Jinja and then by javascript, so needs to be parseable by both.