Allow special characters in auto_accounts by adding data-alias #4673
Conversation
| rm_btn = find("#launcher_auto_accounts_remove_#{acct}") | ||
| add_btn = find("#launcher_auto_accounts_add_#{acct}") | ||
| counter += 1 | ||
| html_acct = "option#{counter}" |
There was a problem hiding this comment.
The counter = 7 here works due to the order of the account fixtures, and the ids are now determined by option order instead of value. I recognize that this is a bit 'magic numbery', but the alternative would be identifying the data-select-value and then finding the add and remove button directly below. That method would be nice for making this test resistant to fixture changes (that might change the index of the options) but would add complexity, and is not really the point of the test. Open to taking the other path though if that is important.
|
|
||
| function cacheAliases(elementId) { | ||
| // This should only run once on each select with an alias defined | ||
| if (!Object.keys(aliasLookup).includes(elementId)) { |
There was a problem hiding this comment.
Can this expression be aliasLookup[elementId] !== undefined instead? It's really not a big deal, there's just something about Object.keys.includes that I feel is wonky in javascript.
That is, the language itself is very flexible in at least trying to access properties. Functions throw errors - but properties never do.
There was a problem hiding this comment.
That makes sense
There was a problem hiding this comment.
Taken care of, with the slight edit that the correct replacement for !Object.keys(aliasLookup).includes(elementId) is aliasLookup[elementId] === undefined
|
|
||
| let optionForValue = mountainCaseWords(document.getElementById(optionForId).value); | ||
| let optionForAlias = ''; | ||
| if ((elementId in aliasLookup) && (value in aliasLookup[elementId])) { |
There was a problem hiding this comment.
value in aliasLookup[elementId]) suggests this is an array? Is it? Seems to me like it should be a scalar string and not an array. And if it is then the expression should be value === aliasLookup[elementId]).
There was a problem hiding this comment.
This solves an issue of scoping the aliases. Especially since we are procedurally generating aliases in auto_accounts, I didn't want there to be any risk of conflict with user-supplied aliases later in the form. So currently aliasLookup has the form {elementId: {value: alias, value2: alias2}}. If we don't like the nesting, we could try the approach of minMaxLookup, which would assemble a unique key from these two parts (and have the form {elementId_value: alias, elementId_value2: alias2}. Would that be preferable?
There was a problem hiding this comment.
and have the form {elementId_alias: value, elementId_alias2: value2}. Would that be preferable?
I see. No I think you've got it. The alternative would require parsing the key with some regular expression that we hope captures all the possible characters.
We just have to be careful here because variable['p-s1.71'] would work but not variable.p-s1.71 wouldn't so we can't access properties by the . operator we have to use the [] operator. Not sure if we do (or have done), just calling it out.
There was a problem hiding this comment.
We just have to be careful here because variable['p-s1.71'] would work but not variable.p-s1.71 wouldn't so we can't access properties by the . operator we have to use the [] operator.
That's a good thing to point out. Maybe it is worth adding a comment above aliasLookup to explain the structure and that detail? I also am realizing I flipped the aliases and value in the above comment. I will correct that but it should be value: alias
There was a problem hiding this comment.
Comments have been added
| let data = $(`${optionSearch}`).filter(function() { | ||
| return (this.value === opt.value); | ||
| }).data(); | ||
| let keys = Object.keys(data).sort(); |
There was a problem hiding this comment.
Can you let me know why this change is needed? Seems like it does the same thing, only sorting at the end.
There was a problem hiding this comment.
The earlier approach broke with the test value <>?/\\:; due to the backslashes. It's a bit confusing to play around with, but I am guessing that the cast in value='${opt.value}' is causing something weird with the backslashes that conflicts with whatever we would need to search using css. By keeping the value comparison in javascript we don't have to cast the values and the same escape patterns operate on this.value and opt.value.
There was a problem hiding this comment.
And the sort is necessary because we need the aliases to be cached before we hit any option-for directives that might need to use it. So sorting just puts the keys in alphabetical order and guarantees that alias comes first.
Fixes #3919 by adding the data-alias directive to dynamic forms. This allows any option-for or exclusive-option-for directive to use a safe name for a value instead of the actual option value. The aliases are scoped to the select widget they are defined in, so the automatically set aliases in auto_accounts will never conflict with a user-defined alias. The alias is only tried if no plain option-for directive is found, so if there were ever a conflict between an alias and a true account value, the true account value would be engaged.
Tests have been added demonstrating the data-alias directive for option-for and exclusive-option-for, and accounts with special characters have been added to the fixtures to ensure the full auto_accounts flow handles these properly. This could be expanded to other auto fields like clusters and queues, but that implementation would closely match what has been done here for account values. The current solution uses aliases for any account with characters beyond digits and lowercase letters, just to be overly cautious.
This PR isn't quite finished, I have not completely resolved the project manager implementation of auto_accounts, so some ProjectManagerTest tests are expected to fail until that is fixed. My plan for those is to identify the options by their order by substituting the value with option{index}. From there
launcher_edit.jsshould be able to match the aliased values by their place in the auto_accounts template. I have less familiarity with the project manager than the dynamic forms, so if this is a flawed approach I would be very welcome to better ideas.I would also like to allow data-max/min-for directives to use aliases, and will be adding that after the project manager stuff is worked out.