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

Allow for custom forms to be edited (+ misc custom form fixes) #3273

Merged
merged 25 commits into from
May 27, 2021

Conversation

willgearty
Copy link
Member

@willgearty willgearty commented Apr 28, 2021

This fixes the ability to edit custom forms through the form builder UI. Apparently this was originally designed to work (my best guess is a django upgrade broke it at some point), but I don't think it's worked for years (if not ever). I've also added links on the custom form landing page to edit a form, which preloads the form for the admin in the form builder. I've tested adding, removing, and editing fields.

I've also used this PR to fix some other custom form issues that have popped up over the years. It's easier than creating a completely new system for the time being (#1842). These include:

Fixes #2959 and fixes #1938.
Also fixes #3256, fixes #1103, fixes #1896, fixes #2904, fixes #2690, fixes #1102, and fixes #792.

@willgearty willgearty changed the title Allow for custom forms to be edited Allow for custom forms to be edited (+ general custom form fixes) Apr 29, 2021
@willgearty willgearty changed the title Allow for custom forms to be edited (+ general custom form fixes) Allow for custom forms to be edited (+ misc custom form fixes) Apr 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented May 1, 2021

This pull request fixes 1 alert when merging 3d544a6 into ce5146c - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 3, 2021

This pull request fixes 2 alerts when merging 7c6d752 into ce5146c - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@willgearty
Copy link
Member Author

OK, I think that's everything that I can find to fix at the moment!

@lgtm-com
Copy link

lgtm-com bot commented May 4, 2021

This pull request fixes 4 alerts when merging 06bc5b4 into ce5146c - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable
  • 1 for Unused local variable
  • 1 for Unused import

Copy link
Contributor

@kkbrum kkbrum left a comment

Choose a reason for hiding this comment

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

Definitely a lot of good changes, but I am having trouble with both adding fields and with linking to programs. It's hard for me to test more components without those, so I'll submit these comments for now and come back to it when the comments about the "add field" and the "multiple modules linked error" get addressed.

esp/public/media/scripts/custom_form.js Show resolved Hide resolved
esp/templates/customforms/index.html Outdated Show resolved Hide resolved
esp/public/media/scripts/custom_form.js Show resolved Hide resolved
esp/esp/customforms/DynamicForm.py Outdated Show resolved Hide resolved
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@willgearty
Copy link
Member Author

OK, I think that addresses all of your comments @kkbrum (and other bugs I found).

@lgtm-com

This comment has been minimized.

Copy link
Contributor

@kkbrum kkbrum left a comment

Choose a reason for hiding this comment

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

Looking better and better! I do have an error when trying to fill out my new form, so that needs to get fixed. The many other comments are truly just suggestions as I looked at every field, I won't reject the PR if they arent addressed or anything!

esp/esp/customforms/linkfields.py Show resolved Hide resolved
esp/public/media/scripts/custom_form.js Show resolved Hide resolved
esp/public/media/scripts/custom_form.js Show resolved Hide resolved
esp/esp/customforms/linkfields.py Show resolved Hide resolved
esp/esp/customforms/views.py Show resolved Hide resolved
esp/esp/users/models/__init__.py Show resolved Hide resolved
esp/esp/users/models/__init__.py Show resolved Hide resolved
esp/esp/customforms/DynamicModel.py Show resolved Hide resolved
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 4 alerts and fixes 5 when merging 8abfe58 into e9842be - view on LGTM.com

new alerts:

  • 3 for Information exposure through an exception
  • 1 for Except block handles 'BaseException'

fixed alerts:

  • 2 for Useless assignment to local variable
  • 1 for Implicit operand conversion
  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com

This comment has been minimized.

@kkbrum kkbrum self-requested a review May 14, 2021 16:00
@willgearty willgearty requested a review from kkbrum May 25, 2021 14:01
@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 4 alerts and fixes 5 when merging 912dfbc into 90b4bc0 - view on LGTM.com

new alerts:

  • 3 for Information exposure through an exception
  • 1 for Except block handles 'BaseException'

fixed alerts:

  • 2 for Useless assignment to local variable
  • 1 for Implicit operand conversion
  • 1 for Unused local variable
  • 1 for Unused import

Copy link
Contributor

@kkbrum kkbrum left a comment

Choose a reason for hiding this comment

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

Monumental!

@willgearty willgearty merged commit dfaff3c into main May 27, 2021
@willgearty willgearty deleted the customform-edit branch May 27, 2021 00:16
willgearty added a commit that referenced this pull request May 27, 2021
willgearty added a commit that referenced this pull request May 27, 2021
* Initial docs for stable release 13

* Docs for #3116, #3117, and #3118

* Added docs about django upgrade

* Docs for #3128

* Docs for #3129, #3133, #3134, and #3137

* Docs for #3156 and #3153

* Docs for #3174, #3163, and #3184

* Docs for #3139, #3140, and #3141

* Docs for #3143, #3150, #3154, #3160, #3162, and #3168

* Docs for #3171, #3185, #3186, and #3188

* Docs for #3131 and #3189

* Docs for #3149 and #3190

* Docs for #3193, #3194, #3195, #196, and #3197

* Clarification

* Docs for #3192, #3201, #3209, and #2248

* Docs for #3204, #3212, #3214, #3205, 9fd073c, and #3226

* Docs for #3232, de5861c, #3231, #3233, #3234, #3237, #3238, and #3239

* Fix indent

* Docs for #3227 and #3235

* Add missing word

* spelling

* Docs for e57581f, #3255, #3253, #3257, and #3249

* Docs for #3254, #3260, and #3262

* Docs for #3263, #3272, #3240, #3264, #3266, and #3270

* clarifications

* Docs for #3283 and #3252

* Docs for #3288 and misc commits

* Docs for #3292, #3311, #3286, #3289, and #3279

* Docs for a377f0d; move note

* Docs for #3315, #3290, and #3322

* Docs for #3273 and #3317

* Final edits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment