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

Revert to ReCAPTCHA 2 for Staticman #45

Merged
merged 8 commits into from Aug 12, 2019

Conversation

@pacollins
Copy link
Owner

commented Aug 10, 2019

Description

ReCAPTCHA reverted back to v2 Checkbox for Staticman

Motivation and Context

Closes #7

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

@pacollins pacollins requested a review from VincentTam Aug 10, 2019

@accesslint
Copy link

left a comment

There are accessibility issues in these changes.

layouts/post/staticman.html Outdated Show resolved Hide resolved
layouts/post/staticman.html Outdated Show resolved Hide resolved
Fixed missing braces to load reCAPTCHA site key
Further testing is going to be carried out to see if #45 works.
@VincentTam
Copy link
Collaborator

left a comment

Simplify config.toml and avoid ...recaptcha.recaptcha since this doesn't look beautiful.

layouts/partials/head.html Outdated Show resolved Hide resolved
layouts/post/staticman.html Outdated Show resolved Hide resolved
static/css/main.min.css Outdated Show resolved Hide resolved
@VincentTam

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

Staticman with reCAPTCHA worked on my demo site: https://framagit.org/vincenttam/fish-demo. @staticmanlab1 managed to push again the master branch of the corresponding repo.

@accesslint
Copy link

left a comment

There are accessibility issues in these changes.

layouts/post/staticman.html Show resolved Hide resolved
layouts/post/staticman.html Show resolved Hide resolved
@pacollins

This comment has been minimized.

Copy link
Owner Author

commented Aug 11, 2019

Now that I have made the change and pushed it. I am torn.

...recaptcha.recaptcha served as a way to simply turn off the feature without deleting the keys. Theoretically, you could just comment out the keys now I suppose.

If we are doing this, should we change ...staticman.staticman to a similar system of {{ if and ...}}? The new method would be backwards compatible in theory (assuming people only fill in the information if they want to use the feature).

{{ else if .Site.Params.staticman.staticman }}
<article class="post">
{{ .Render "staticman" }}
</article>
{{ end }}

@VincentTam

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

Now that I have made the change and pushed it. I am torn.

Thanks for editing. It's Sunday, so you may take a rest and lemme do the testing and reviewing work for you.

...recaptcha.recaptcha served as a way to simply turn off the feature without deleting the keys. Theoretically, you could just comment out the keys now I suppose.

Even though this theme is called "Future Imperfect", when we look into the past, that's really a breaking change. But what's broken? Here's the only case.

    [params.staticman.recaptcha]
      recaptcha = false
      siteKey           = "<nonempty site key>" # Site Key
      encryptedKey      = "<nonempty encrypted key>"

Here're the repos using this theme with recaptcha = false: https://github.com/search?o=asc&q=theme+%3D+%22hugo-future-imperfect-slim%22+AND+%22recaptcha+%3D+false%22+filename%3Aconfig.toml+fork%3Atrue&s=indexed&type=Code. I haven't (and perhaps won't) check all of them, but from the samples that I've viewed, all of them has simply left this section untouched—perhaps due to error #7.

To sum up, we're breaking a broken stuff (so as to reassemble and fix it), so we don't need to worry about this.

If we are doing this, should we change ...staticman.staticman to a similar system of {{ if and ...}}? The new method would be backwards compatible in theory (assuming people only fill in the information if they want to use the feature).

{{ else if .Site.Params.staticman.staticman }}
<article class="post">
{{ .Render "staticman" }}
</article>
{{ end }}

Lemme do that for you. From the last previous part, we can forget about the backward compatibility.

Upcoming: some Staticman changes out of the scope of this PR.

ℹ️ In the next time, when the local branch and the remote branch have diverged for just a few commits, please use git pull --rebase to avoid leaving a merge commit at the head of an unmerged PR. This linearise the commit graph of the branch.

Simplify code for calling Staticman
In the same spirit of ac8c52a.

@VincentTam VincentTam self-assigned this Aug 11, 2019

VincentTam added a commit to VincentTam/hugo-future-imperfect-slim that referenced this pull request Aug 11, 2019

VincentTam added a commit to VincentTam/hugo-future-imperfect-slim that referenced this pull request Aug 11, 2019

Fixed missing braces to load reCAPTCHA site key
Further testing is going to be carried out to see if pacollins#45 works.

VincentTam added a commit to VincentTam/hugo-future-imperfect-slim that referenced this pull request Aug 11, 2019

Squashed commit of the following:
commit ff52ad0
Author: Vincent Tam <VincentTam@users.noreply.github.com>
Date:   Sun Aug 11 10:14:52 2019 +0200

    Simplify code for calling Staticman

    In the same spirit of ac8c52a.

commit 220e0f5
Merge: 5ccfc71 7ed1b06
Author: pacollins <thepatrickcollins@gmail.com>
Date:   Sun Aug 11 01:06:12 2019 -0400

    Merge branch 'Revert-staticman-recaptcha'

commit 5ccfc71
Author: pacollins <thepatrickcollins@gmail.com>
Date:   Sun Aug 11 01:03:58 2019 -0400

    Revert socnet deletion

commit ac8c52a
Author: pacollins <thepatrickcollins@gmail.com>
Date:   Sun Aug 11 01:01:48 2019 -0400

    Remove redundancy of recaptcha.recaptcha

commit 320c0d8
Author: pacollins <thepatrickcollins@gmail.com>
Date:   Sun Aug 11 01:01:24 2019 -0400

    Remove extra article tag

commit 7ed1b06
Author: Vincent Tam <VincentTam@users.noreply.github.com>
Date:   Sun Aug 11 02:04:04 2019 +0200

    Fixed missing braces to load reCAPTCHA site key

    Further testing is going to be carried out to see if pacollins#45 works.

commit 8484671
Merge: 1ce3d53 f0ec5ac
Author: Patrick Collins <thepatrickcollins@gmail.com>
Date:   Fri Aug 9 21:45:43 2019 -0400

    Merge branch 'master' into Revert-staticman-recaptcha

commit 1ce3d53
Author: pacollins <thepatrickcollins@gmail.com>
Date:   Fri Aug 9 21:39:01 2019 -0400

    Revert to ReCAPTCHA 2 for Staticman
@VincentTam

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

🙏 Thanks for your great work this w.e. 🔬 I've just tested it again on my test site on Framagit 🌐 served by the Framagit instance @staticmanlab1. It worked !!!

📝 Test report [TL;DR]

  1. Squash merged recent commits into my dev branch.

    Compare my dev branch with your Revert-staticman-recaptcha branch: https://github.com/pacollins/hugo-future-imperfect-slim/compare/Revert-staticman-recaptcha..VincentTam:dev?expand=1

    SHA-1 hash of my dev branch: 5a3e756

  2. Updated the submodule of my test site.

    SHA-1 hash of the master branch: 18df4ac

  3. Submitted a comment at a sample post.

  4. Redirected to the post. @staticmanlab1 commited directly into the master branch.

    SHA-1 hash of the master branch: 19fb99d. Its parent is the commit mentioned in step 2. Note that the config.toml has not yet been changed. This shows the backward compatibility of this PR at ff52ad0.

  5. Git pulled the Staticman comment in step 3 into my local repo.

  6. Removed the lines recaptcha = true and staticman = true in config.toml.

    diff --git a/config.toml b/config.toml
    index 51fc23e..e3a379e 100644
    --- a/config.toml
    +++ b/config.toml
    @@ -92,7 +92,6 @@ disableLanguages        = [""]
         # Sets Statiman to be active
         # If using GitHub, go to https://github.com/apps/staticman-net
         # If using GitLab, just add the GitLab bot, NO need to hit `/connect/v3/...`
    -    staticman           = true
         # Sets the location for Staticman to connect to
         api                 = "staticman-frama.herokuapp.com"  # without trailing slash
         gitProvider         = "gitlab"  # either "github" or "gitlab"
    @@ -101,7 +100,6 @@ disableLanguages        = [""]
         branch              = "master"
     
         [params.staticman.recaptcha]
    -      recaptcha         = true
           siteKey           = "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo" # Site Key
           encryptedKey      = "GLGBV5a+DHijkgBrPXRVvlK2CUrgyo0vOgpnVFTX76VUsTtrxqK8X5mJ9IjXiZbSmEwpflr6il1aLPueoV38nv9ZBIbPNUzj1xmt1CQ9HUOQFpB+n22HlwKJPqj29X8xkgCKnR190WK/KpPau6rF/PYzQn/eI+6hEEcCfOWUm9iQqUSk9vpCTtFD7LnFtxd+TjCxfY8R0uFpvs141aOAv/8wcR3ZzOjoWx3adAQkAgu6QLC1SVBQVnqDKiBrw4Tys/P8LhJE2Ul28tItI7XH5ZUpt8AV/38t5v+6cMEAk3sUr1VqnrPfjnQrLysXQlyeHMqqV48IZLLBWuMb0OqGog=="

    SHA-1 hash of this commit: 62c5562

  7. Submitted a comment again at the post mentioned in step 3.

  8. @staticmanlab1 received the comment, and regenerated the site at commit 3c1d2d9, whose parent is the commit mentioned in step 6.

@VincentTam VincentTam referenced this pull request Aug 11, 2019
2 of 7 tasks complete

@pacollins pacollins merged commit d80bfe5 into master Aug 12, 2019

1 check was pending

AccessLint Reviewing for accessibility issues

@pacollins pacollins deleted the Revert-staticman-recaptcha branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.