luci-app-ddns: show the URL which will be used. #7559
Conversation
|
This can also be considered as a fix for #5297. |
|
In #3582 folks suggest that LuCI should parse the URL for the If we don't plan to do that (or do that right now) and we start to show the URL in the UI with this PR, could we make all the currently-mandatory fields optional? And basically make it the responsibility of the user to fill in the fields used in the URL, based on the hints in under the fields? |
|
I suppose this is a start at being helpful, but it does have exceptions. Some providers have old info that mandates a username+password, but providing a token (i.e. password field) only, works fine. The providers template URLs drive which fields are mandated. Improvements in this area are welcome. |
|
Would you prefer this PR to also attempt to do those improvements, or would it make sense to accept the current work to expose the URL, and to subsequent work in a separate PR? |
|
We're close to 24 release, so minimal (atomic) changes are the easiest to test and review. But this can be kept minimal if done right (mandating fields based on those in the URL). The field presence in the provider URL is fairly easy to detect. It might require an exhaustive review of providers and their URLs (which are all in another repo anyway). This PR as-is satisfies the issue already, no? |
|
If we are talking about the #2436, I believe this PR as-is should significantly alleviate hardship people might experience attempting to fill the form to match the expectations, by showing where each of the values will land. |
| @@ -674,6 +680,11 @@ return view.extend({ | |||
| }; | |||
| } | |||
|
|
|||
| if (Boolean(s.url)) { | |||
| o = s.taboption('basic', form.DummyValue, '_url', _("Update URL")); | |||
| o.default = s.url; | |||
There was a problem hiding this comment.
I find that monospace makes the URL easier to parse:
o.rawhtml = true;
o.default = '<div style="font-family: monospace;">' + s.url; + '</div>'There was a problem hiding this comment.
We'd have to properly escape that s.url as well.
If we want to show this in monospace, I'd much rather add support for .monospace to form.Value and form.DummyValue and configure it more cleanly that way, rather than hacking it with o.rawhtml. (A much bigger change, granted.)
There was a problem hiding this comment.
I had a thought that perhaps adding a whole .style element might be the way to go. But that's for another day.
The whole point of .rawhtml is so you don't have to escape. That you provide raw HTML.
https://openwrt.github.io/luci/jsapi/LuCI.form.DummyValue.html
There was a problem hiding this comment.
Right. And if we are to provide raw HTML, it has to be a valid, properly escaped HTML, doesn't it? And we cannot know that for the URL value.
So I added some .replaces; rebased on master.
There was a problem hiding this comment.
It seems we have escape functions scattered all over the place, so having some central function would be useful.
In this case, I think we should also handle at least ' and " also.
There was a problem hiding this comment.
Those only have to be escaped in attributes.
Signed-off-by: Jan Pazdziora <[email protected]>
Signed-off-by: <[email protected]>row (viagit commit --signoff)<package name>: titlefirst line subject for packagesPKG_VERSIONin the MakefileReplaces [DOMAIN] in Update-URL (URL-encoded)comments. It's certainly nice to have that information but I always wondered "what is that Update-URL?" for this service? Then I came across luci-app-ddns: Namecheap DDNS setting are misleading #2436 which basically discusses that this DDNS provider does not use username and expects host (?) instead. We likely cannot finetune the description of the fields in the UI to this case but maybe if we actually showed the admin the Update-URL which will be used, it might be easier for them to figure out which field to use for what.