Fix code quality issues (AI findings)

[DominicBM]

Summary

• application_controller.rb: Guard CGI.unescape with is_a?(String) to handle non-string param values; use admin? in admin_for_all_hubs? to guarantee boolean return (admin && returns nil instead of false when the column is nil)
• devise.rb: Remove commented-out secret_key and pepper values (sensitive data in VCS); remove Devise 4.x Warden no-op strategy workaround — now on Devise 5.0.3
• metadata_completeness.rb: Rename sThree_response → s3_response (inconsistent camelCase); fix comment typo "data" → "date"
• contributors/index.html.erb: Simplify element.checked == true → element.checked

Not actioned

• redirect_to "#{request.path}?..." flagged as open redirect — false positive; request.path is always the server's own /-prefixed path and cannot redirect to an external domain
• current_end_date missing params[:end_date].present? guard — intentional; @end_date is always set by DateSetter (defaults to today), so the asymmetry with current_start_date is by design

Test plan

• Auth flow works (Devise 5.x Warden workaround removal)
• admin_for_all_hubs? returns false (not nil) for non-admin users
• Metadata completeness pages load correctly (s3_response rename)

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved parameter handling to correctly process string-encoded values
  • Updated authorization verification for admin access checks

• Refactor

  • Optimized cloud storage CSV retrieval mechanism
  • Simplified client-side column toggle logic
  • Removed obsolete authentication configuration options

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR updates application controller parameter handling and authorization checks, renames an S3 helper method across metadata completeness calls, simplifies boolean logic in a view template, and removes unused Devise configuration entries and strategy block.

Changes

Code Modernization and Configuration Cleanup

Layer / File(s)	Summary
Authorization API app/controllers/application_controller.rb	admin_for_all_hubs? calls current_user.admin? instead of the current_user.admin predicate.
Parameter Handling app/controllers/application_controller.rb	normalize_encoded_params guards CGI unescaping to only apply when the parameter value is a String, not based on general truthiness.
S3 Helper Refactoring app/lib/metadata_completeness.rb	Internal helper renamed from sThree_response to s3_response; both hub_csv and contributor_csv caches call the updated method. Min date comment updated.
View Logic app/views/contributors/index.html.erb	toggle_column condition simplified to if (element.checked) instead of if (element.checked == true).
Devise Configuration config/initializers/devise.rb	Removes commented-out config.secret_key and config.pepper lines and deletes the config.warden no-op strategy handler block for recoverable, trackable, and validatable.

Possibly related PRs

• dpla/dashboard-analytics#305: Adjusts the same ApplicationController#normalize_encoded_params method introduced in that PR, adding stricter String type checking.
• dpla/dashboard-analytics#280: Both PRs modify the S3 response helper in metadata_completeness.rb, with this PR renaming the method and the other fixing error handling within it.
• dpla/dashboard-analytics#265: Modifies the same admin_for_all_hubs? authorization check in ApplicationController.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A dash of cleanup, method by method,
Parameters now checked—strings are respected!
S3 calls properly named with care,
Devise configs simplified everywhere. 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Fix code quality issues (AI findings)' is vague and generic, using non-descriptive language that doesn't clearly convey the specific changes made in the changeset.	Replace with a more specific title that highlights the main change, such as 'Fix type guards and method calls in controllers, lib, and views' or focus on the most impactful change like 'Remove Devise 4.x workaround and fix Warden configuration'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/ai-quality-findings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

app/controllers/application_controller.rb (1)

63-68: 💤 Low value

Inconsistent usage: require_admin! still uses current_user.admin.

For consistency with the fix at line 50, consider updating this method to also use admin?:

 def require_admin!
-  unless current_user.admin
+  unless current_user.admin?
     flash[:alert] = "You don't have permission to do that."
     redirect_to admin_user_path(current_user)
   end
 end

While the current code works (nil is falsy), using admin? consistently across the codebase improves clarity and avoids potential confusion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/controllers/application_controller.rb` around lines 63 - 68, The
require_admin! method currently checks current_user.admin; update it to call
current_user.admin? for consistency with the change at line 50 and to use the
predicate style; modify the require_admin! method (the redirect/flash logic
remains the same) so the conditional uses current_user.admin? instead of
current_user.admin.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@app/controllers/application_controller.rb`:
- Around line 63-68: The require_admin! method currently checks
current_user.admin; update it to call current_user.admin? for consistency with
the change at line 50 and to use the predicate style; modify the require_admin!
method (the redirect/flash logic remains the same) so the conditional uses
current_user.admin? instead of current_user.admin.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c01cbf0-8e30-42b5-9d89-fb7b5bcf141d

📥 Commits

Reviewing files that changed from the base of the PR and between a5be117 and 8dbdbfc.

📒 Files selected for processing (4)

• app/controllers/application_controller.rb
• app/lib/metadata_completeness.rb
• app/views/contributors/index.html.erb
• config/initializers/devise.rb