Fixes #10017: Improve data sources UI #1460
Conversation
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
from
57ad4d0
to
7f47351
Compare
|
Commit modified |
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
from
7f47351
to
355d906
Compare
|
PR rebased |
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
from
355d906
to
152151f
Compare
| r = r.replace(new RegExp("[ùúûü]", 'g'),"u"); | ||
| r = r.replace(new RegExp("[ýÿ]", 'g'),"y"); | ||
| r = r.replace(new RegExp("\\W", 'g'),"_"); | ||
| $scope.selectedDatasource.id = r; |
There was a problem hiding this comment.
Isn't their a function in JS to map UTF-8 space to ascii? (+take care of ascii space, at least this one is enumerable).
Because, we could do nothing, it would be least surprising than here (for ex, what about the "ğ" ?
And why limit key space? It could be full UTF-8 chars (trimed), no?
There was a problem hiding this comment.
OK, js is just a ridiculus language. But that requirement should appear in a lot of place in our code, no?
If so, having an external function like: https://github.com/backbone-paginator/backbone.paginator/blob/a579796a30e583c4dfa09e0a86e4abd21e0b5b56/plugins/diacritic.js would be better.
There was a problem hiding this comment.
I don't find any Javascript native function to map UTF-8 space to ASCII.. But maybe we can use a more complete solution like this ?
https://github.com/Kepro/angular-remove-diacritics
There was a problem hiding this comment.
Oops I didn't see your previous answer. The Angular Service I linked is precisely based on this function :)
There was a problem hiding this comment.
Some more thinking: there is no other keys in Rudder that are not UTF-8 ;
Other node properties can be UTF-8 ;
It is not enforced by API (so that we can add data sources with utf-8 id in all case);.
So I'm really not sure that we want to enforce that only in the web UI (I'm pretty sure of the opposite).
@jooooooon @VinceMacBuche what do you think?
There was a problem hiding this comment.
I agree with @fanf, did some research too, and found the same library ... we should not do ourselves something someone else already did !!
There was a problem hiding this comment.
if some validation needs to be done on keys/id we need to do them in backend (prevent invalid characters, being valid identifier ...)
checking on UI side gives a better UX but validating on backend side seems more important
There was a problem hiding this comment.
Policy generation fails when you use a property based on utf8 characters
Indeed cfengine arrays does only support cfengine valid ([a-zA-Z_]+) keys, we should valideate properties keys likweise
There was a problem hiding this comment.
Replacing invalid key characters (not "[-_a-zA-Z0-9]") by _, is a better solution
|
apart utf8 remparks this is all good |
|
PR rebased |
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
from
ede3c74
to
d301912
Compare
|
Commit modified |
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
2 times, most recently
from
65d8283
to
5da01c3
Compare
|
Commit modified |
| <form class="bloc-body show-details" name="forms.datasourceForm"> | ||
| <div class="form-group"> | ||
| <label for="name">Name</label> | ||
| <input type="text" class="form-control input-sm" id="name" ng-value="selectedDatasource.name" name="datasourceName" required ng-model="selectedDatasource.name" > | ||
| <input type="text" class="form-control" id="name" ng-value="selectedDatasource.name" name="datasourceName" required ng-model="selectedDatasource.name" ng-change="updateKeyName(selectedDatasource.name)"> |
There was a problem hiding this comment.
You may want this to be trimmed (using ng-trim)
There was a problem hiding this comment.
By default, this option is activated. ng-trim allows us to set it to false if necessary but it is not our case :)
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
from
5da01c3
to
23c5724
Compare
|
PR rebased |
| "updateTimeout":{"day":0,"hour":0,"minute":5}, | ||
| "requestTimeout":{"day":0,"hour":0,"minute":5} | ||
| "schedule":{"second":0,"minute":5}, | ||
| "updateTimeout":{"second":0,"minute":5}, |
| "requestTimeout":{"day":0,"hour":0,"minute":5} | ||
| "schedule":{"second":0,"minute":5}, | ||
| "updateTimeout":{"second":0,"minute":5}, | ||
| "requestTimeout":{"second":0,"minute":5} |
| "modifiedTimes":{ | ||
| "schedule":{"day":0,"hour":0,"minute":5}, |
There was a problem hiding this comment.
Here, by default, every hour is OK.
|
Commit modified |
RaphaelGauthier
force-pushed
the
ust_10017/improve_data_sources_ui
branch
from
23c5724
to
4826f2c
Compare
|
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/10017