Skip to content
This repository has been archived by the owner on Jun 9, 2021. It is now read-only.

Add the ability to specify forms for buttons #173

Merged
merged 1 commit into from
Dec 25, 2016

Conversation

itay
Copy link
Contributor

@itay itay commented Dec 23, 2016

We had a dire need to be able to get some interactivity from forms when pressing buttons, so that we could, for example, decide what kind of unit tests to run or builds to create.

At first I was thinking that it would be nice to have a form builder in the UI itself, but I'm way too lazy and not that talented, so instead I opted to allow you to specify a JSON representation of a form and then we'd just render it properly.

For example, this form:

[
        {   "name": "var1",
            "label": "var1 label",
            "defaultValue": "you can put a variable like this: ${PULL_REQUEST_AUTHOR_NAME}",
            "type": "input", 
            "required": false,
            "description": "var1 description"
        },
        {   "name": "var2",
            "label": "var2 label",
            "defaultValue": "any string can go here",
            "type": "textarea", 
            "required": false,
            "description": "var2 description"
        },
        {   "name": "var3",
            "label": "var3 label",
            "defaultValue": "option2_name",
            "options": [
                {"label": "option1 label", "name": "option1_name"},
                {"label": "option2 label", "name": "option2_name"},
                {"label": "option3 label", "name": "option3_name"}
            ],
            "type": "radio", 
            "required": true,
            "description": "var3 description"
        },
        {   "name": "var4",
            "label": "var4 label",
            "type": "checkbox", 
            "required": true,
            "options": [
                {"label": "option1 label", "name": "option1_name", "defaultValue": true}, 
                {"label": "option2 label", "name": "option2_name", "defaultValue": true}
            ],
            "description": "var4 description"
        }
]

Ends up looking like this: rendered_form

And you can edit it like this:
image

In the form you can also use the normal variables (modulo BUTTON_FORM_DATA of course) so that they will be rendered as well. The form will then be sent to the server using jQuery's serializeArray format. For the above form, it looks like this:

{
   "var1":"you can put a variable like this: admin",
   "var2":"any string can go here",
   "var3":"option2_name",
   "var4":[
      "option1_name",
      "option2_name"
   ]
}

I specified it in the URL as such, but you can also put it as POST data of course: ?form_data=${BUTTON_FORM_DATA}

I fixed up the units so they're all passing, and I also handled HTML escaping of the content to make sure nothing terrible happens. In terms of improvements that are possible:

  1. It would be nice if the server gave you better errors beyond "this has to be valid JSON", but I didn't want to make it parse too much. A form builder would do a better job here of course by guaranteeing correct input.
  2. Would probably be good to have more tests.
  3. I'm not 100% sure about how I ended up escaping things when I render the variable for JSON, but I could not think of a better way.

Hope this makes sense and happy holidays 🎄

@tomasbjerre
Copy link
Owner

I'll have a deeper look into this when I have the time, but I will definitely accept this PR! =)

About 3. I added a JQuery plugin that adds serializeJSON. There is also a plugin there that adds populate to populate the form from JSON.

Squashing would be nice, but its up to you. I see you changed the CHANGELOG.md a bit. But perhaps you saw that its auto generated from GIT.

@itay
Copy link
Contributor Author

itay commented Dec 24, 2016

I will fix up to use the JSON plugin as well as cleanup a few other things including the changelog.

Hopefully will have an updated PR tomorrow with these changes.

This change adds the ability to specify a JSON-based form for a given
button, which will get automatically rendered when the button is pressed.
The submitted data is available as serialized JSON in the ${BUTTON_FORM_DATA}
variable.

For the specification of what a form looks like and it's serialized result,
look at README.md in the change.
@itay
Copy link
Contributor Author

itay commented Dec 24, 2016

OK - should be much improved - a single commit with a reasonable commit message, I took out CHANGELOG.md, as well as cleaned up a few small bugs. It also adds documentation to the README.

@tomasbjerre tomasbjerre merged commit 96f3584 into tomasbjerre:master Dec 25, 2016
@tomasbjerre
Copy link
Owner

Thanks!

tomasbjerre added a commit that referenced this pull request Dec 25, 2016
 * Adding fmt-maven-plugin to format code on Google Java Format.
 * Correcting warnings from Grunt / JSHint.
 * Adding, and using, enum, RENDER_FOR, in PrnfbRenderer.
@tomasbjerre
Copy link
Owner

I browsed the code a bit and I have some thoughts.

Perhaps its better to have DTO:s that represent the buttonForm. Perhaps ButtonFormDTO and it can have a list of ButtonFormOptionDTO.

That would enable us to do better checks on the JSON provided here. So that we could have errors like No name supplier or Type not recognized...

We would also just need to render the defaultValue here.

We would have JSON already so we dont need to parse here.

I added ENCODE_FOR where JSON would be removed and we can use NONE instead. And let the framework encode the defaultValue string to JSON correctly.

Let me know your opinion or if you start working on that. I will try to do it when I find the time.

@tomasbjerre
Copy link
Owner

I'm looking into this now =)

tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Storing the button form in objects, not a String.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Storing the button form in objects, not a String.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Storing the button form in objects, not a String.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Storing the button form in objects, not a String.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
tomasbjerre added a commit that referenced this pull request Dec 26, 2016
 * Triggering without showing dialog, if no form supplied.
 * Adding DTO:s to properly validate JSON in form.
 * Storing the button form in objects, not a String.
 * Adjusting textarea size in GUI.
 * Supplying form as JSON to GUI, instead of escaped JSON string.
@tomasbjerre
Copy link
Owner

I think I'm happy with the implementation now. Will do some more testing before release.

tomasbjerre added a commit that referenced this pull request Dec 27, 2016
@tomasbjerre
Copy link
Owner

This is now added to 2.44.

tomasbjerre added a commit that referenced this pull request Dec 27, 2016
 * Quotes were escaped with slashes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants