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

Form Submission Cookie Not Automatically Setting "Form" Key #866

Closed
fpcorso opened this issue Aug 4, 2020 · 3 comments
Closed

Form Submission Cookie Not Automatically Setting "Form" Key #866

fpcorso opened this issue Aug 4, 2020 · 3 comments

Comments

@fpcorso
Copy link
Contributor

fpcorso commented Aug 4, 2020

Describe the bug

When generating any trigger and automatically adding a "Form Submission" cookie, the cookie gets created but the form key does not get set. This causes the cookie to not be set. The issue is resolved by editing the cookie and either making no changes or setting the form to any form. Once updated, the form key will be valid.

Site information

Popup Maker version: 1.11.1
WordPress version: 5.4

Expected behavior

Should automatically set the form key.

Current behavior

form key is not set.

Steps to reproduce

  1. Create popup
  2. Create any trigger, leaving "Prevent popup from showing to visitor again using a cookie?" checked and "Form Submission" as the cookie.
  3. Save popup
  4. Review values to see no form key is present:
    image
  5. Edit the cookie but make no changes. Click "Update" on cookie modal. Save popup.
  6. Review values to see the new form key:
    image

Errors

No JavaScript errors are shown during cookie creation or editing.

@fpcorso fpcorso added this to the v1.12 milestone Aug 25, 2020
@fpcorso
Copy link
Contributor Author

fpcorso commented Sep 1, 2020

@danieliser I think this may affect any cookie that has any extra settings added. In triggers.js, we check to see if the checkbox is checked and set the time, path, and event type here: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/popup-editor/plugins/triggers.js#L431

Which we then pass to insertCookie over in cookies.js which only extends to make sure there is an event type and a name: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/popup-editor/plugins/cookies.js#L97

That then gets added to the editor through the row.add method in the same file: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/admin/popup-editor/plugins/cookies.js#L201

Unless I am mistaken, I don't think Popup Maker ever adds in any default settings for any extra setting field that is added to a cookie, such as form in this case. Is there anything I might have missed here?

If this is correct, it should be as simple as doing a cookie = cookies.get_cookie(event) and then merging the values in cookie.fields in, right? Maybe do that in the insertCookie method to catch any cookies added from anywhere that is missing the defaults?

@danieliser
Copy link
Member

@fpcorso - It is one of the older systems still in place, very well possible it has no default value merge.

Try it and see what happens is all we can do.

@danieliser danieliser reopened this Sep 2, 2020
fpcorso added a commit that referenced this issue Sep 4, 2020
@fpcorso
Copy link
Contributor Author

fpcorso commented Sep 4, 2020

@danieliser After a bit more debugging, I realized there are other settings that haven't been included too due to this. For example, the session-wide setting was never set for any cookie if you never manually edited the cookie. Of course, our checks also checked if it was set so it didn't affect anything unless they went in and turned it on so it wasn't an issue anyone would notice.

So, I set up a getCookieDefault method which flattens the default fields into the settings object and then used that for the extend instead of our hardcoded one. After a bit of testing, looks like this solves the issue and has no side-effects with any other cookie creation or editing.

@fpcorso fpcorso closed this as completed Sep 4, 2020
@fpcorso fpcorso mentioned this issue Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants