add Gateway teaser page for CE users - #1593
Conversation
etdsoft
left a comment
etdsoft
left a comment
There was a problem hiding this comment.
Summary
This PR adds a Gateway teaser page for CE users following the established "show don't gate" pattern. The implementation is clean overall, but there's a crash path from an unguarded Project.find(1), a global .card-title margin override that causes visual regression risk across the app, an unscoped Activity query, and missing tests for the new helper methods.
Findings
F1 [High|sad-path] app/controllers/static_pages_controller.rb:23 — Project.find(1) crashes if project 1 does not exist
What: set_gateway_projects calls Project.find(1), which raises ActiveRecord::RecordNotFound (500 error) if no project with ID 1 exists. This is the first static teaser page that performs a real database lookup — Issue Library and Remediation Tracker use only mock data.
Why: While CE conventionally has a single project with ID 1, this isn't guaranteed after a DB reset, re-seed with different auto-increment, or if the default project was deleted. The page becomes the only "show don't gate" page that can 500 while the others render fine.
How to fix: Use Project.first instead of Project.find(1) — it's more resilient and semantically correct (CE has one project, use it). Add a guard for the nil case:
def set_gateway_projects
@project = Project.first
return redirect_to root_path unless @project
@projects = [
{ name: @project.name, ... }
] + gateway_projects
endF2 [Medium|correctness] app/assets/stylesheets/hera/modules.scss:118 — global .card-title margin override causes app-wide regression risk
What: The new rule .card .card-title { margin-bottom: 0; } removes bottom margin from every .card-title inside a .card in the entire application — affecting comments, boards, subscriptions, issues empty-state actions, and more.
Why: This is broader than needed. The existing issuelib teaser already solved this locally with <h4 class="card-title mb-0"> (a Bootstrap utility class on the element). The global override silently changes spacing on ~10 unrelated views and makes the issuelib's mb-0 class redundant. Card titles in comments, boards, and subscriptions may now render with incorrect spacing.
How to fix: Remove the .card-title block from modules.scss and add mb-0 to the gateway card title, matching the issuelib pattern:
<h4 class="card-title mb-0">Projects</h4>F3 [Medium|correctness] app/controllers/static_pages_controller.rb:30 — unscoped Activity.maximum(:created_at) queries all projects
What: set_gateway_projects uses Activity.maximum(:created_at) to populate the updated_at field for the projects table row. This queries the entire activities table, not scoped to project 1. Meanwhile, the active project card in the view correctly uses @project.activities.maximum(:created_at).
Why: If multiple projects exist in CE (created via upload or setup), the table row will show a "last updated" date from a different project's activity, while the card above it shows the correct project-scoped date. The same project could display two different "last updated" values on the same page.
How to fix: Scope the query to the project:
updated_at: @project.activities.maximum(:created_at) || Date.todayF4 [Medium|tests] app/helpers/static_pages_helper.rb:0 — no tests for helper methods with non-trivial query logic
What: StaticPagesHelper introduces four methods — chart_styles, issues_grouped_by_tag, top_issues_by_evidence_count, and top_nodes_by_issue_count — with LEFT JOIN, GROUP BY, custom Arel.sql ordering, and conditional logic. None have tests. The gateway_index controller action and request spec are also absent.
Why: These methods contain real database query logic (joins, grouping, aggregation) and branching (nil tag handling, zero-evidence fallback). Unlike the hardcoded mock data in tickets and issuelib_entries, these run live queries against project data. A future schema change or association rename could silently break them. The PR description notes these were ported from the gateway engine where they were "tested and peer-reviewed" — but those tests don't exist in CE.
How to fix: Add specs covering the helper methods and the gateway_index route. At minimum: top_issues_by_evidence_count orders by evidence count and handles zero evidence; top_nodes_by_issue_count orders by distinct issue count and excludes non-user nodes; issues_grouped_by_tag groups by first tag sorted by position and places untagged last; gateway_index renders successfully and handles empty projects.
F5 [Low|consistency] app/assets/stylesheets/hera/modules/_active_project.scss:2 — dead CSS for .avatars and .donuts sections
What: The stylesheet includes ~100 lines of styles for .avatars (lines 2–34) and .donuts (lines 45–106) that are never used in any CE view. The PR description states these sections were "omitted as they have no CE equivalent," but the latest commit (sync _active_project.scss with Pro for CE-Pro parity) brought them back.
Why: Dead CSS increases bundle size and maintenance surface. The .more-gravatars rule also has a duplicate border-radius: 50% declaration (lines 25 and 27), suggesting a copy-paste from Pro without cleanup.
How to fix: Either remove .avatars and .donuts blocks from the CE version (matching the original intent), or update the PR description to explain why syncing the full Pro stylesheet is preferred. If keeping them, remove the duplicate border-radius on line 27.
F6 [Low|consistency] app/views/static_pages/gateway_index.html.erb:38 — raw HTML anchor tag instead of link_to
What: <a href="https://dradis.com/tools/gateway.html" target="_blank">Dradis Gateway</a> uses a raw HTML tag.
Why: CLAUDE.md convention: "Always use Rails helpers — never raw HTML tags for assets or links."
How to fix: <%= link_to 'Dradis Gateway', 'https://dradis.com/tools/gateway.html', target: '_blank' %>
F7 [Low|hygiene] CHANGELOG:5 — changelog entry missing future-tense verb
What: The changelog entry is just Gateway under the Show don't gate section, with no verb describing what the upgrade does.
Why: CLAUDE.md changelog rules: "Feature entries: description starts with a future tense verb — frame it as 'What will this upgrade do to my instance?'" The existing Node-level Methodologies entry has the same issue, but extending a pattern that doesn't match the documented convention is worth flagging.
How to fix: Change to something like:
- Show don't gate:
- Gateway: add teaser page with live project data
- Node-level Methodologies
Notes
- The
Arel.sql(...)calls inStaticPagesHelperuse static strings only (no interpolation), so they're safe from SQL injection. - The hardcoded
Project.find(1)andprojects/1/addons/gatewayroute follow the exact same pattern as the existing issuelib and remediation tracker teasers. Thefind(1)pattern itself is the concern, not the route design. - The
js-try-proclass is used as a JS hook throughout the CE codebase (~20 occurrences). While CLAUDE.md conventions preferdata-behaviorselectors, this is a deeply established pattern — refactoring it is a separate task.
3a7c56c to
aef9151
Compare
2 participants
Summary
Adds a static Gateway teaser page for CE users, accessible under Tools > Gateway in the navbar. The page shows:
This follows the "show don't gate" pattern used for Issue Library and Remediation Tracker — CE users see a real, functional preview of the feature rather than a locked-out placeholder.
How to test
bin/dev) and log in as any user/projects/1/addons/gatewayjs-try-proupsell modal (not a navigation)Dradis::Prois definedOther Information
StaticPagesHelperportschart_styles,top_issues_by_evidence_count,top_nodes_by_issue_count, andissues_grouped_by_tagfrom the gateway engine'sactive-projects/hide-empty-contentbranchI assign all rights, including copyright, to any future Dradis work by myself to Security Roots.
Check List