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

Restrict access to pages based on cookies/form submissions #162

Open
wants to merge 6 commits into
base: staging
Choose a base branch
from

Conversation

joshdarby
Copy link

@joshdarby joshdarby commented May 12, 2020

Changes

This pull request makes the following changes:

  • Adds a new checkbox metabox to the editor for posts and pages to toggle whether or not the content should be restricted behind a cookie.
  • Adds a new set_cookie_on_form_submission function that sets a cookie for 365 days in the users browser once they submit a form if the form contains a hidden field with access_ID as its label
  • Adds a new restrict_access_to_pages_by_cookie function that restricts access to a page if the aforementioned checkbox is checked. However, if the user has a cookie set named unrestrict_POST_ID (POST_ID will be an actual post id) and it's value is set to true, the page will show.
    • If user lands on a restricted page and doesn't have a cookie set, the form will show. Once filled out, user is redirected to the page with content shown.
    • If user is on a restricted child page, it looks to see if a cookie is set to grant access to its top level parent page. If it is, access is granted
    • If a user is on a restricted child page and no top level parent cookie is set, it redirects the user to the top level parent page to fill out the form.

Notes:

  • The name of the form must match the top level parent page that is restricted.
  • In order to know what post the form grants access to, a hidden field must be placed as the last field in the form and the value needs to be the post ID of the post to grant access to and the label must be access_ID

Restrict access metabox:
Screen Shot 2020-05-12 at 1 54 57 PM

Top level page with form:
ezgif-3-3ecde6edf709

Page with no matching form but that is restricted:
Screen Shot 2020-05-12 at 1 54 37 PM

Why

For #161

Testing/Questions

Features that this PR affects:

  • Pages and posts that enable the Restrict Access checkbox

Questions that need to be answered before merging:

  • Is this PR targeting the correct branch in this repository?
  • Any logic I missed?
  • Any other edge cases we need to watch out for?

Steps to test this PR:

  1. Create a new page or pick a page to test with
  2. Enable the "Restrict Access" checkbox
  3. Create a new GF form
    • make the title of the form the exact title of your test page
    • make the last field of the form a hidden field and give it the value of your test page and make the label access_ID
      Screen Shot 2020-05-12 at 2 06 23 PM
  4. Add a child page to your test page and restrict it.
  5. Add a child page to your child page and restrict it.
  6. Open your test page in an incognito window and fill out the form. Hopefully you're redirected to the page with the content now and are able to also view the child pages.

@joshdarby joshdarby requested review from benlk and kaylima May 12, 2020 18:00
@joshdarby joshdarby self-assigned this May 12, 2020
Comment on lines +189 to +202
// but wait, what if this is a child page?
// we need to see if a cookie has been set to grant access to the parent page
// first, grab the top-level parent of this page
if( $post->post_parent ) {

$ancestors = get_post_ancestors( $post->ID );
$root = count( $ancestors ) - 1;
$parent = $ancestors[$root];

} else {

$parent = $post->ID;

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than checking the immediate parent, should we check instead whether the cookie's ID is in the array of ancestors?

Copy link
Author

Choose a reason for hiding this comment

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

We could do that. In your opinion, what would make that more beneficial than this way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The possibility of a metered page having a grandchild or great-grandchild page. They're already using great-grandchild pages: https://learn.inn.org/startup-introduction/chapter-1-where-to-start-in-nonprofit-news/creating-a-business-model/idea-generation/

@benlk
Copy link
Collaborator

benlk commented May 12, 2020

Two implementation-detail questions:

  • How does this work if WPE has cached the page HTML? Does WPE in production environments serve different page results with and without the cookie?
  • Is it more future-proof if the form name is the ID of the post instead of the name of the post? That way, folks could rename the post without breaking the form.

@joshdarby
Copy link
Author

Two implementation-detail questions:

  • How does this work if WPE has cached the page HTML? Does WPE in production environments serve different page results with and without the cookie?

That's a great question that I'm not sure about until we put it up on a WPE environment and test it out. We may possibly need to add some cachebusting into it.

  • Is it more future-proof if the form name is the ID of the post instead of the name of the post? That way, folks could rename the post without breaking the form.

I'm torn between yes and no here. Yes, the ID would be better in a lot of ways. But then we'll have users looking through x amount of forms trying to find the correct one because they don't know the actual ID of the page/post form they're looking for, and I'm not sure if it's a reasonable ask to expect them to know the ID.

@benlk
Copy link
Collaborator

benlk commented May 13, 2020

But then we'll have users looking through x amount of forms trying to find the correct one because they don't know the actual ID of the page/post form they're looking for, and I'm not sure if it's a reasonable ask to expect them to know the ID.

We could add to the meta box a link to the appropriate form. In pseudocode:

$form_exists = false;
for post_id in array( this post ID, ancestor post IDs ) {
    if form_exists( $post_id ) {
        $form_exists = true;
        "This post's visibility would be controlled by <this form>"
    }
}
if ( ! $form_exists ) {
   "no form exists for this post. [create one](/wp-admin/admin.php?page=gf_new_form) with the title $post_id"
}

There doesn't appear to be a way to prefill the form title in the create-new-form form, unfortunately. I was hoping &new_form_title=12345 or &title=12345 would prefill it with "12345", but the plugin contains no logic to do that:

https://github.com/wp-premium/gravityforms/blob/ca668e0dc28bc42a8634be619837bbdc8f445d16/form_list.php#L96

@joshdarby
Copy link
Author

But then we'll have users looking through x amount of forms trying to find the correct one because they don't know the actual ID of the page/post form they're looking for, and I'm not sure if it's a reasonable ask to expect them to know the ID.

We could add to the meta box a link to the appropriate form. In pseudocode:

$form_exists = false;
for post_id in array( this post ID, ancestor post IDs ) {
    if form_exists( $post_id ) {
        $form_exists = true;
        "This post's visibility would be controlled by <this form>"
    }
}
if ( ! $form_exists ) {
   "no form exists for this post. [create one](/wp-admin/admin.php?page=gf_new_form) with the title $post_id"
}

There doesn't appear to be a way to prefill the form title in the create-new-form form, unfortunately. I was hoping &new_form_title=12345 or &title=12345 would prefill it with "12345", but the plugin contains no logic to do that:

https://github.com/wp-premium/gravityforms/blob/ca668e0dc28bc42a8634be619837bbdc8f445d16/form_list.php#L96

@benlk I like this idea, but the inability to populate the form title with the post ID makes me weary because then we're back at the question of whether or not its a reasonable ask to expect the user to know how to find the post ID :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants