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

[JENKINS-43326] Invalid (randomly) form submission JSON from optional blocks ("groupNode") #2858

Closed
wants to merge 3 commits into from

Conversation

tfennelly
Copy link
Member

@tfennelly tfennelly commented Apr 25, 2017

See JENKINS-43326.

@recampbell @varmenise

Guys, if you take a look at the commits you'll see the steps I went through.

TL;DR: some piece of async code is resetting some behaviours.js added properties on the optionalBlock element (actually looks as though the element is being recreated - didn't manage to find what code is doing this), causing the form submission value to be screwed when the form is being submitted (because the form submission code does not think the checkbox is relating to a "groupNode").

To fix, I changed the behaviour.js code and form submission code to set and get a DOM attribute value instead. This seems to work from the testing I've done i.e. I could see that the groupNode property on the e was being lost, while the attribute value was not being lost.

Of course, needs more testing and checking etc.

…eRef) is somehow getting reset after the fact

The groupingNode property on the element is getting set to true via the "TR.row-set-end" behaviour, but *sometimes* is lost from the element by the time the form is submitted.

A listener/timer is cutting in async from somewhere it seems hence the groupingNode prop is being lost
.. and it was ... suggesting something is createating the element from scratch => losing the groupingNode property
… attribute.

The attribute values do get copied i.e. not lost by whatever is modifying the element.
@tfennelly tfennelly added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Apr 25, 2017
@tfennelly tfennelly changed the title [JENKINS-43326] [JENKINS-43326] Invalid (randomly) form submission JSON from optional blocks ("groupNode") Apr 25, 2017
@tfennelly
Copy link
Member Author

Looks like the form-element-path-plugin would be effected by this change. Maybe we need to continue setting the element property for a while.

See form-element-path.js

@recampbell recampbell requested a review from abayer April 25, 2017 15:54
Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, but honestly, I have no idea about any of this code. =)

Copy link
Member

@stephenc stephenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and aligns with my analysis for form submission related code I developed in the LDAP plugin

@@ -1339,7 +1339,7 @@ function replaceDescription() {
* and attached under the element identified by the specified id.
*/
function applyNameRef(s,e,id) {
$(id).groupingNode = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have seen the exact same random recreation of elements in other code and my fix was to move storage to a different location. In my case I couldn't stash in an attribute because YUI was doing other changes to the dom.

My interpretation was that there is a hashmap like data structure and hence the hash-order is non-predictable leading to some of the time getting the element recreated

@recampbell
Copy link
Member

@Vlatombe @olivergondza Can one of you comment on whether this will break form-element-path-plugin as Tom fears?

@jglick
Copy link
Member

jglick commented May 3, 2017

Unless you patch form-element-path to match (say by looking for either idiom), it is likely that acceptance-test-harness will be broken.

@recampbell
Copy link
Member

recampbell commented May 4, 2017

🐛 When I tested this, the descriptor radio boxes didn't show up.

I think I have a reproduction of the actual issue and am working on filing it.

@vilacides
Copy link

Hi @tfennelly The last feedback on this PR is that it's broken and that was on 2017. Are you planning to work on this? If not, are you OK with closing the PR? it is consuming CI cicles. Thanks!

@daniel-beck
Copy link
Member

This seems abandoned for two years now based on the comment, so closing it. It seems like a useful change though, and it would be nice if you would resubmit it for review.

@daniel-beck daniel-beck closed this Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-hold This pull request depends on another event/release, and it cannot be merged right now
Projects
None yet
7 participants