-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add support for Staticman reCAPTCHA #224
Conversation
Great PR! Any chance to weight this in? |
@lobopraveen Would you mind if I change/rearrange some config options before merging? |
Feel free to modify any way you see fit.
… |
Three new configs have been introduced: 1. enableReCaptcha 2. reCaptchaSiteKey 3. reCaptchaSecret Last two configs are the same as `reCaptcha > siteKey` and `reCaptcha > secret` from the staticman.yml
I've removed the options from Can you confirm that it works? |
Due to eduardoboucas/staticman#235 and eduardoboucas/staticman#243 (comment), please make it clear (say, by editing the title of this PR), that this concerns reCAPTCHA v2. |
@MunifTanjim Thanks for update. These lines in 57277aa are a smart way to avoid setting the reCAPTCHA parameters twice. I've never seen that before in other Jekyll/Hugo themes supporting reCAPTCHA. minimo/layouts/partials/comments/staticman/form.html Lines 11 to 12 in 57277aa
However, calling parameters from Between convenience of setting |
The way I see it, Also, it is required to exist if you are using Staticman. So, I can't think of any problem reading
It is left to the theme. If the user is using Staticman, Again, the
Can you please give me any example about what could go wrong in the future? 🤔 |
@MunifTanjim Let's get out of the box and think about the possibility of an alternate commenting system with reCAPTCHA support (say, from Staticman to Stapsher). Setting it in the site config is the most convenient way enable the user to re-use the key in case of switching commenting system. Note that it's common to switch commenting system in case of its dysfuctionning, for example, eduardoboucas/staticman#260 (comment). Putting reCAPTCHA's parameter inside
Thanks for clarification, but that doesn't affect my main point: a clear separation of roles of theme-specific site config files ( |
I'm confused 😕 If the theme doesn't know if the user is using Disqus/Staticman, how would it render the template for those?
Staticman API service is only concerned about the
I skimmed through the docs for Minimal Mistakes and their approach is different. The docs tell you to put all the options from
"other themes do it" not a good reason to just follow the same approach. Also, the reason behind not reading I don't know if Jekyll has the equivalent function for Hugo's With Hugo, we can!
Staticman encrypts the reCaptcha Secret with their own private key. Setting it in
I don't think I get your point. Say somebody's using Staticman. According to your suggestion, they will have to set the reCaptcha config in both Accoding to my suggestion, when using Staticman, user puts reCaptcha config in |
@MunifTanjim Lemme say it in a better way. It's the user's choice to use whatever commenting system they like. A theme does care the user's choice, but its theme owner doesn't. I've mixed up the two.
Where? You're referring to the deprecated Staticman v1? That's not the case since v2.
My bad. My mind had blocked me.
Who knows? I suppose we aren't fortune tellers. By the time you asked this question, somebody reported error in eduardoboucas/staticman#302, which is caused by a breaking change in Staticman's authorisation mechanism with GitHub. Due to such uncertainty, we need a clear division of roles, so that things are maintainable. |
( Not talking about staticman's _config but the theme's
I don't see how that issue is related to this. If you choose to use Staticman, you have to deal with the problems that come with it. If Staticman someday shuts down parmanently, you will still have your static comment files in your repository. You can either run your own Staticman instance or migrate to another commenting system. But other commenting systems won't support the same format that Staticman uses. So, you will need to change the formats of those static comment files yourself. That's just how it is...
I have no idea why you keep bringing up "clear division of roles" in your comments or what exactly you mean by that... I don't know if you've read through the whole source codes of Minimo and noticed how it's organized. Without knowing about how different parts of Minimo fits together, it's easy to be confused about things. From all our discussions, I still don't see any downsides of reading Upsides:
Downsides:
So, unless you or somebody else who is interested in using Minimo & Staticman together can point out exactly what the downsides of reading |
The link is about Staticman v2. Just underneath the sample Staticman repo config, there's a site config file, saying that
So I can't understand why you say
Nobody would read the whole code---they'll only read the relevant part. I don't want to be self-advertising, but I did clone your project to Framagit, and its P.S. It's me who has written the section "staticman v3" above your linked Minimal Mistakes docs, that's why I'm a little bit shocked about your remarks on Minimal Mistake's
The second point can never be true. Can you find another theme that put all Staticman's configurations in a single file? That's impossible. In a theme template, Staticman's configurations consist of configurations for sending, receiving and rendering data. The first task requires several parameters in the URL (e.g. Git service, Git user name, etc) that can't be put under |
Ah, I see, sorry about that. I looked at that link and https://github.com/mmistakes/minimal-mistakes/blob/master/_config.yml and got confused. My bad 😅
Yeah, I know. That's why this exists: minimo/exampleSite/config.toml Lines 97 to 103 in 4436676
By Staticman's configuration, I meant the options that are supported in |
Edited: I mean a distinction between the theme-specific parameters and theme non-specific ones.
|
If you're really concerned about users' convenience, I would suggest you to add a reCAPTCHA section at the bottom of Staticman's config file. # reCAPTCHA (OPTIONAL)
# Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
# Use your OWN siteKey and secret.
reCaptcha:
enabled: false
#siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
# (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
# i.e. https://staticman3.herokuapp.com/v3/encrypt/{your-site-secret}
# For more information, https://staticman.net/docs/encryption
#secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw==" At the first glance, this shows that reCAPTHCA is supported. reCAPTHCA users can uncomment the two parameters to get the service, and other users can simply let it untouched. That would reder For your second upside, I think you meant to say "reCAPTCHA's config", taking your "Staticman's configuration" into account.
Having read your lines again, I have finally come up with a drawback from a pratical point of view. minimo/layouts/partials/comments/staticman/form.html Lines 11 to 12 in 57277aa
For active blogs like zcrc.me, it often receives comments from @staticmanlab. Every time a comment gets merged, the site is re-built. It would be more sensible to load only two parameters from config.toml instead of the entire file staticman.yml .
Besides, in the template file, the Hugo variable {{ if .Site.Params.staticman.recaptcha }}
<input type="hidden" name="options[reCaptcha][siteKey]" value="{{ .Site.Params.staticman.recaptcha.sitekey }}">
<input type="hidden" name="options[reCaptcha][secret]" value="{{ .Site.Params.staticman.recaptcha.secret }}">
{{ end }} The site and Staticman configurations are supposed to be done once only, until the next theme/Staticman update. On the other hand, site regeneration happens each time the site/theme content/static files/etc change, so that takes place much more frequently. The inconvience of pasting the parameters twice in a short-term downside, but the additional blog regeneration time is a long-term one. |
I absolutely agree with the first part of your suggestion. I'm gonna update the sample But the second part 🤔 well, let's be realistic... We all know how fast Hugo is... So, for small sites, the time needed to read and parse a file multiple times will not even be noticeable. It will probably be noticeable if the site has thousands of pages... but then again, with a site that big, who's gonna use Minimo 😂 For the sake of argument, let's say somebody uses Minimo with a very large site, then we can just separate the read-and-parse-file part of the template in a separate partial and cache that partial. So it will actually be only read and parsed once per build (not once per page). Hugo supports caching and returning data from partial. So it'll not be a problem.
Edit: Well, scratch that last line. Let's implement that caching part right in this PR! 😃 |
@VincentTam That should solve the performance concern... |
@MunifTanjim Big template for Minimo site! 😲
After you've put Staticman v2 URL, I know how that's related to this PR, which modifies the documentation for Staticman. That link PR reports the breaking of Staticman v2 on the public production API instance, while your instructions ask people to continue using @staticmanapp. In fact, there's many Staticman on GitHub. Some of them might be a bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First time I try GtiHub review
@MunifTanjim Please stop propagating the errors reported in eduardoboucas/staticman#222, eduardoboucas/staticman#227 and eduardoboucas/staticman#302, which have resulted in the use of the official production Staticman instance.
Please update the your theme's docs like Minimal Jekyll, Beautiful Jekyll & Hugo Swift Theme to provide info about Staticman v3 URL scheme introduced in eduardoboucas/staticman#219, which Staticman's official docs don't cover. If you feel that the docs are too heavy, at least, for the sake of user's convenience, add a commented line in Staticman config template like the following. # reCaptcha
# Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
reCaptcha:
enabled: false
#siteKey: "6LdRBykTAAAAAFB46MnIu6ixuxwu9W1ihFF8G60Q"
# Encrypt reCaptcha secret key using Staticman /encrypt endpoint
# For more information, https://staticman.net/docs/encryption
#secret: "PznnZGu3P6eTHRPLORniSq+J61YEf+A9zmColXDM5icqF49gbunH51B8+h+i2IvewpuxtA9TFoK68TuhUp/X3YKmmqhXasegHYabY50fqF9nJh9npWNhvITdkQHeaOqnFXUIwxfiEeUt49Yoa2waRR7a5LdRAP3SVM8hz0KIBT4=" source: eduardoboucas/staticman#243 (comment) You've spent effort in inventing a method for reading two parameters from a YML file for users' convenience during site setup. Why don't you spare some time to put in a one-line comment to give users a better idea on these two parameters? # reCAPTCHA (OPTIONAL)
# Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
# Use your OWN siteKey and secret.
reCaptcha:
enabled: false
#siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
# (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
# i.e. https://{your-api}/v3/encrypt/{your-site-secret}
# For more information, https://staticman.net/docs/encryption
#secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw==" With minimal effort, everyone knows that
|
@@ -8,6 +8,9 @@ | |||
|
|||
{{- $api := ( print $apiEndpoint "/" $username "/" $repo "/" $branch ) -}} | |||
|
|||
{{- $staticman := ( partialCached "comments/staticman/GetStaticmanYML" . ) -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to avoid duplicate input, then branch
should be treated equally as reCaptcha
parameters.
{{- $branch := $staticman.branch -}}
I'm regreting my decision to include Staticman support in this theme 🙃 |
With all due respect to Vincent, you decide what you want to incorporate
from his suggestions. Although he has some valid points, it is you who will
have to maintain this project so you make the ultimate call.
I don't remember all the discussions but from what I recall I'd put the
flag to enable/disable the staticman in theme's config and read rest from
the staticman config like you did at first. I'd also put a link to the
staticman documentation instead of writing it in your project. It is not
this project's problem to solve if staticman documentation is not up to
date.
…On Sat, Jul 6, 2019, 8:32 AM Munif Tanjim ***@***.***> wrote:
I'm regreting my decision to include Staticman support in this theme 🙃
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#224?email_source=notifications&email_token=AAKR3HIEAXLKW6TW5M4U6J3P6CNGHA5CNFSM4HK5BC5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZKZXFQ#issuecomment-508926870>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKR3HK54MGA4YVI7AJGX3DP6CNGHANCNFSM4HK5BC5A>
.
|
@MunifTanjim Why? You've done a great job in providing the Staticman integration. Honestly speaking, your Hugo skills and logic used in this theme are much more advanced than other Jekyll/Hugo themes I've seen. I've created a Staticman + Minimo + Framagit demo site based on your amazing work: https://framagit.org/staticman-gitlab-pages/minimo. Just a bit of documentation and modification in the template are needed to enhance it and to avoid Staticman's issues. I should have mentioned my changes to the docs here: https://staticman-gitlab-pages.frama.io/minimo/docs/comments-support/#framagit. The benefits of static comments are numerous, and you're probably more familiar with them than I do. Nonetheless, I would like to mention the fact the Disqus (along with popular Western IT companies like Google, Facebook, etc) is blocked in the PRC, which has several hundred millions of netizens. You might know more from the GitHub repo cirosantilli/china-dictatorship. To make the static blogs static, but resposive, not just in terms of mobile-friendliness, but in terms of its ability to interact with its readers, a static commenting system is a great-to-have. You've put huge effort in developing an integration that excels that of Network Hobo, which contains a logical error mentioned the repo's first issue. This great feature has already provided convenience to Staticman users. Please don't be discouraged by the recent errors and difficulities that I've brought up during the discussion because Staticman is a distributed commenting system that would give users total control over their comments. By distributed, it means everyone can has its own version. That would free users in case of central server being blocked (due to maybe political reasons). I've written my experience to free some interested users from @staticmanapp, whose v2 no longer accepts invitation, contrary to what the official documentation claims. Others have also written their own versions, based on their perception on Staticman. Looking back, I should have used less "should" and find an alternative word for that.
@lobopraveen Your code speaks for itself. In what way did you load parameters from minimo/layouts/partials/comments/staticman/form.html Lines 45 to 58 in b72b3bd
That's why we need to be clear about the role played by different config files. If we don't make it clear, we might commit errors, which might mislead users by any means of communication.
The theme is named "Minimo". Would you expect users who want minimal setup to
You've made great effort on the logical side, but please think from the users' point of view.. After failing to set up a correct URL, desperate users might try whatever they can think of. If they get a little bit of hint from our theme, which aims at providing convenience for site owners, they might spare more time to create more meaningful stuff in return. I've learnt a great deal from the developer of Staticman's native GitLab support. This has enabled me to create demos which helps others to understand in turn. There're growing number of Staticman bot on GitHub/GitLab. To sum up, don't ignore a little butterfly in our documentation. They might have a far-reaching impact on static site owners who wish to create great contents. |
Everything that's in scope of this PR is covered already. Merging... |
Three new configs have been introduced:1. enableReCaptcha2. reCaptchaSiteKey3. reCaptchaSecretLast two configs are the same as
reCaptcha > siteKey
andreCaptcha > secret
from the staticman.ymlJust set the
reCaptcha
configs in yourstaticman.yml
file.Fixes #220
Although this works, you may want to refactor the JavaScript introduced in
layouts/partials/comments/staticman/form.html
. I would do it but I don't know how the JavaScript is all packaged intostatic/assets/js/comments.1234.js
The COMMENT! submit button should be disabled on click of the REPLY button on a comment or the CANCEL button (after click on REPLY). This is because the reCAPTCHA checkbox gets cleared on those clicks. If I check the reCAPTCHA and then click on REPLY or CANCEL, I can still submit the comment even though the reCAPTCHA checkbox is disabled. This is not a problem as I would have proved that I'm human by clicking on it once already but it just looks odd.