Issue 53306: Some LKS forms don't distinguish between fields properly #6804
Conversation
…g only by special chars
| @@ -215,12 +215,6 @@ public boolean isShouldLog() | |||
| return delegate.isShouldLog(); | |||
| } | |||
|
|
|||
| @Override | |||
| @@ -721,7 +721,7 @@ public JSONArray getIssues() | |||
| /** | |||
| * Parse out the issues forms from the JSON data | |||
| */ | |||
| public List<IssuesForm> getIssueForms() | |||
| public List<IssuesForm> getIssueForms(User user, Container container) | |||
There was a problem hiding this comment.
nit: Initializing a shared object here _issueForms with non-constant values for user and container could result in an improper result.
There was a problem hiding this comment.
nit: Initializing a shared object here
_issueFormswith non-constant values for user and container could result in an improper result.
Which objects are shared?
There was a problem hiding this comment.
The _issueForms is shared but created given variable input.
There was a problem hiding this comment.
But IssuesApiForm is allocated per request.
There was a problem hiding this comment.
Maybe your point is that User and Container don't need to be passed in at all?
There was a problem hiding this comment.
My point is that there are multiple calls to getIssueForms(User user, Container container) that currently all pass in the same container/user. However, if one did not, and it was different, say an admin user or something, it could end up returning an instance that is not initialized with the admin user but then continuing work assuming it is.
There was a problem hiding this comment.
Right, which is why we have getViewContext() for Action classes.
There was a problem hiding this comment.
I'll make IssuesApiForm non-static so it can get the user and container from the ViewContext. This more clearly binds it to an individual request.
| "priority", "1", | ||
| Maps.of("Type", "UFO", | ||
| "Area", "Area51", | ||
| "Priority", "1", |
There was a problem hiding this comment.
Why is "Comments" capitalized in mothership but then "comments" here in issues? Does matching case actually matter I guess is what I'm really asking.
There was a problem hiding this comment.
I don't love the mix, but the issues form is custom-coded. It now has a mix of camel and capitalized fields, depending on whether they're lookups or not. Mothership is generated by a more standard update form codepath.
Yes, plus for making script-friendly names in a couple of places in report code. I moved it from ColumnInfo to PageConfig and renamed it. I didn't attempt to make them all unique though. See my most recent commit in this FB. |
Rationale
Some
<input>fields are usingnameattributes that don't differentiate from similarly named fields.Changes
ColumnInfo.getPropertyName()