-
Notifications
You must be signed in to change notification settings - Fork 662
AO3-6198 Use empty strings for Akismet info in config #5542
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi, varram! Thank you so much for this pull request. Someone will be along to review it soon. Unfortunately, Jira has been unreliable about showing us new accounts in the admin panel lately, and the link you provided says it's for a user that doesn't exist. If you could either reply here or send an email to otw-coders@transformativeworks.org with your email address, we'll make sure the account gets set up right. (Once it's set up, you'll have the ability to comment on, assign, and transition issues.) While we get that straightened out, I've updated the Jira issue status to In Review so no one mistakenly creates a duplicate pull request. Thanks again for contributing! If you have any questions, you can contact us at the same email address listed above. |
|
Hi @sarken ! Thanks for the quick reply! Here’s the email address associated with my Jira account: Appreciate you moving the issue to In Review while this gets sorted out. Please let me know if you need anything else from me. Cheers, |
|
Your Jira permissions should be all set the next time you log in! |
Bilka2
left a comment
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.
Thanks for working on this! In addition to my review comment, please also action the Rubocop comments
| validate :check_for_spam, on: :create | ||
|
|
||
| def check_for_spam | ||
| return unless %w(staging production).include?(Rails.env) |
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.
The spam? method already skips spamchecking if we're not in Staging or Production, so this check is redundant. Could you please remove it?
(I think removing this should also fix the tests)
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-6198
Purpose
What does this PR do?
This PR clarifies Akismet configuration and ensures spam checking only runs in staging/production environments.
Changes made:
AKISMET_KEYandAKISMET_NAMEvalues inconfig.ymlwith empty strings to make it clear they're not actual valid credentialscheck_for_spammethods inComment,Feedback, andAbuseReportmodels to only execute in staging and production environments (matching the existing pattern in theWorkmodel)This prevents spam checking from running in development/test environments where Akismet credentials aren't configured, while maintaining existing functionality in staging and production.
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
varram (he/him)
P.S. This is my first PR (planning on doing many more!) so please let me know if I need to do anything else for this :).
Also Here is my jira account (for permissions? 🤞)