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

Stop url encoding in UI templates #20

Merged
merged 8 commits into from Feb 19, 2021

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Feb 9, 2021

(See corresponding issue.)

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x requested a review from nhoening Feb 9, 2021
@Flix6x Flix6x self-assigned this Feb 9, 2021
@Flix6x Flix6x linked an issue Feb 9, 2021 that may be closed by this pull request
@Flix6x Flix6x added bug UI labels Feb 9, 2021
Copy link
Contributor

@nhoening nhoening left a comment

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

@Flix6x
Copy link
Contributor Author

@Flix6x Flix6x commented Feb 10, 2021

@Flix6x Flix6x assigned nhoening and unassigned Flix6x Feb 16, 2021
@Flix6x
Copy link
Contributor Author

@Flix6x Flix6x commented Feb 16, 2021

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
Copy link
Contributor

@nhoening nhoening commented Feb 16, 2021

@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
Copy link
Contributor

@nhoening nhoening commented Feb 17, 2021

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
Copy link
Contributor

@nhoening nhoening commented Feb 17, 2021

@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
Copy link
Contributor Author

@Flix6x Flix6x commented Feb 18, 2021

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.

@Flix6x Flix6x assigned Flix6x and unassigned nhoening Feb 18, 2021
@Flix6x Flix6x changed the title Stop url encoding in two places. Stop url encoding in UI templates Feb 19, 2021
@Flix6x Flix6x merged commit f961fde into main Feb 19, 2021
2 checks passed
@Flix6x Flix6x deleted the issue-19-Bug_broken_link_on_asset_page_for_some_assets branch Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants