diff --git a/.agent_rules.md b/.agent_rules.md new file mode 100644 index 00000000..d3e83662 --- /dev/null +++ b/.agent_rules.md @@ -0,0 +1,331 @@ +# Rules for AI + +## 1. Code Quality + +- **Follow Rubocop**: Always consider our Rubocop rules when writing code. If uncertain about a style or convention, check `.rubocop.yml` or existing code patterns. + +## 2. Techniques & Standards + +### Ruby Style + +**Use Hash Value Omission**: For Ruby 3.1+ code, use hash value omission syntax when the key and value are the same. + +- **BAD**: `{ instance: instance, duration: duration }` +- **GOOD**: `{ instance:, duration: }` +- **BAD**: `create(:account, instance: instance, status: status)` +- **GOOD**: `create(:account, instance:, status:)` + +This applies to: + +- Method arguments +- Hash literals +- Factory definitions +- Any hash where keys match variable names + +**Avoid Ugly Multi-line Formatting**: When array or hash elements span multiple lines, put each element on its own line. Never create partial multi-line elements that result in awkward `,],` or `,),` patterns on the same line. + +- **BAD** - partial multi-line creates ugly `,],`: + + ```ruby + rows: [ + ['Label', format_number(value), + format_change(value, previous),], + ] + ``` + +- **GOOD** - each element on its own line: + + ```ruby + rows: [ + [ + 'Label', + format_number(value), + format_change(value, previous), + ], + ] + ``` + +- **ALSO GOOD** - single line if short enough: + ```ruby + rows: [ + ['Label', format_number(value), format_change(value, previous)], + ] + ``` + +Note: Trailing commas are fine (and enforced by Rubocop). The issue is the _formatting_ that creates visual clutter. + +### ActiveRecord Queries + +**Use Ruby Ranges**: Prefer Ruby range syntax over string comparisons for date/time and numeric ranges. + +- **BAD**: `.where('created_at > ?', 1.day.ago)` +- **GOOD**: `.where(created_at: 1.day.ago..)` (Use endless range for `>=`) +- **GOOD**: `.where(created_at: ..1.day.ago)` (Use beginless range for `<=`) +- **GOOD**: `.where(created_at: ...1.day.ago)` (Use range with 3 dots for `<`) + +### Exception Handling + +Always specify the exception class in rescue clauses: + +- **BAD**: `rescue => e` +- **GOOD**: `rescue StandardError => e` +- **BETTER**: `rescue SpecificError => e` + +Let errors propagate naturally unless you have a specific reason to catch them. + +### Database migrations + +Always use `rails generate migration` to create new migrations. Avoid creating migration files manually to ensure proper timestamps and naming conventions. + +## 3. Architecture + +**No Side Effects in Models**: Avoid putting side-effects (like sending notifications or enqueuing jobs) in models. These belong in the Service Layer / Interactors. + +## 4. Testing + +### Validation Testing + +**DO NOT** write specs that test basic ActiveRecord validations like: + +- `validates :field, presence: true` +- `validates :field, uniqueness: true` +- `validates :field, numericality: true` +- Other standard Rails validations + +#### Why? + +- These validations are tested by Rails itself +- They add maintenance overhead without adding value +- They make specs brittle to simple model changes + +#### What to test instead: + +- Complex custom validations with business logic +- Validation behavior in integration/request specs +- The actual user-facing behavior that relies on validations + +#### Examples: + +**BAD** - Testing simple validations: + +```ruby +describe Beacon do + it { is_expected.to validate_presence_of(:api_key_digest) } + it { is_expected.to validate_uniqueness_of(:api_key_prefix) } +end +``` + +**GOOD** - Testing through behavior: + +```ruby +describe "POST /beacons" do + subject(:request) do + post beacons_url, params: { beacon: { language_id:, region_id: } } + end + + let(:language_id) { create(:language).id } + let(:region_id) { create(:region).id } + + it "creates a beacon" do + expect { request }.to change(Beacon, :count).by(1) + + expect(response).to redirect_to(beacon_url(Beacon.last)) + end +end +``` + +**GOOD** - Testing complex custom validations: + +```ruby +describe "custom validation" do + subject(:tag_cognate) { described_class.new(tag:, cognate:) } + + context "when tag and cognate are the same" do + let(:tag) { create(:tag) } + let(:cognate) { tag } + + it "prevents self-reference" do + expect(tag_cognate).not_to be_valid + expect(tag_cognate.errors[:cognate]).to include("can't be the same as tag") + end + end +end +``` + +### Mocks and Spies + +Prefer using `instance_spy` and the `expect(...).to receive` style for verification over `have_received`. This mocking style is generally more readable and separates setup from verification effectively. + +Use `instance_spy` primarily when you stub the initializer to return the spy. This allows you to avoid defining the interface of a double explicitly, as spies allow any message. + +**BAD**: + +```ruby +let(:generator) { instance_double(Beacons::ApiKeyGenerator, call: nil) } + +before do + allow(Beacons::ApiKeyGenerator).to receive(:new).and_return(generator) +end + +it "generates an API key" do + subject.call + expect(generator).to have_received(:call) +end +``` + +**GOOD**: + +```ruby +let(:generator) { instance_spy(Beacons::ApiKeyGenerator) } + +before do + allow(Beacons::ApiKeyGenerator).to receive(:new).and_return(generator) +end + +it "generates an API key" do + expect(generator).to receive(:call) + subject.call +end +``` + +#### For Class Methods + +For class methods, set expectations directly before the action - do not use `allow` + `have_received`. + +**BAD**: + +```ruby +before do + allow(FileWorker).to receive(:perform_async) +end + +it "enqueues file processing job" do + action.call + + expect(FileWorker).to have_received(:perform_async).with( + topic.id, language.id, anything, + ) +end +``` + +**GOOD**: + +```ruby +it "enqueues file processing job" do + expect(FileWorker).to receive(:perform_async).with( + topic.id, language.id, anything, + ) + + action.call +end +``` + +### Use Contexts for Conditional Scenarios + +Always use `context` blocks to organize specs by conditions or states. This makes specs more readable and clearly separates different scenarios. + +**Use `context` for:** + +- Different states: `context 'when user is authenticated'` +- Different inputs: `context 'with valid params'` +- Different configurations: `context 'with feature flag enabled'` +- Error conditions: `context 'when API fails'` +- Variations: `context 'with force: true'` + +**BAD**: + +```ruby +describe "#call" do + it "returns error for revoked beacon" do + # ... + end + + it "processes active beacon" do + beacon.update!(revoked_at: nil) + # ... + end + + it "sends notification when provider is present" do + # ... + end +end +``` + +**GOOD**: + +```ruby +describe "#call" do + context "with revoked beacon" do + let(:beacon) { create(:beacon, :revoked) } + + it "returns an error without processing" do + # ... + end + end + + context "with active beacon" do + let(:beacon) { create(:beacon) } + + it "processes the request" do + # ... + end + + it "associates topics with beacon" do + # ... + end + end + + context "when file generation fails" do + before do + allow(XmlGenerator::SingleProvider).to receive(:call).and_raise(StandardError) + end + + it "captures exception and returns error" do + # ... + end + end +end +``` + +**Benefits:** + +- Clear separation of different scenarios +- Easier to locate specific test cases +- Better test organization and readability +- `let` blocks can be scoped to specific contexts + +### Side-Effect Records in Before Blocks + +When creating records that exist only for their side effects (not directly referenced in tests), use `before` blocks instead of `let!`. This keeps `let` blocks clean and focused on records that are actually referenced. + +**BAD** - Creating side-effect records in `let!`: + +```ruby +let!(:provider_with_topics) do + provider = create(:provider) + create(:topic, provider:, state: :archived) + provider +end +``` + +**GOOD** - Separate the side-effect into a `before` block: + +```ruby +let!(:provider) { create(:provider) } + +before do + create(:topic, provider:, state: :archived) +end +``` + +**Why?** + +- `let` blocks should return the record they're named after +- Side effects hidden inside `let` blocks are harder to understand +- `before` blocks clearly signal "setup that happens before tests" +- Makes test data setup more explicit and readable + +## 5. Workflow + +- **Run DB migrations**: Make sure to always run any pending database migrations with `rails db:migrate RAILS_ENV=test` command before running tests. +- **Run Verification**: Once a chunk of work is completed, always run both Rubocop checks and relevant tests to ensure no regressions or style violations. diff --git a/.gitignore b/.gitignore index 12846bc1..b29899bd 100644 --- a/.gitignore +++ b/.gitignore @@ -43,3 +43,7 @@ db/structure.sql /app/assets/builds/* !/app/assets/builds/.keep + +# Ignore key files for decrypting credentials and more. +/config/*.key + diff --git a/app/models/current.rb b/app/models/current.rb index 5725e6e2..cc0f42c2 100644 --- a/app/models/current.rb +++ b/app/models/current.rb @@ -1,5 +1,6 @@ class Current < ActiveSupport::CurrentAttributes attribute :session attribute :beacon + attribute :feature_overrides delegate :user, to: :session, allow_nil: true end diff --git a/app/models/feature_flags.rb b/app/models/feature_flags.rb new file mode 100644 index 00000000..394b3a23 --- /dev/null +++ b/app/models/feature_flags.rb @@ -0,0 +1,62 @@ +module FeatureFlags + extend self + + def enabled?(flag_name, user: nil) + override = Current.feature_overrides&.dig(flag_name.to_sym) + return override unless override.nil? + + env_value = ENV["FEATURE_#{flag_name.to_s.upcase}"] + return cast_boolean(env_value) unless env_value.nil? + + config = feature_config(flag_name) + return false if config.nil? + return cast_boolean(config) unless config.respond_to?(:dig) + + return true if cast_boolean(config[:enabled]) + + user_allowed?(config, user) + end + + def disabled?(flag_name, user: nil) + !enabled?(flag_name, user: user) + end + + def enable!(flag_name) + (Current.feature_overrides ||= {})[flag_name.to_sym] = true + end + + def disable!(flag_name) + (Current.feature_overrides ||= {})[flag_name.to_sym] = false + end + + def with(flag_name, value) + old = Current.feature_overrides&.dig(flag_name.to_sym) + value ? enable!(flag_name) : disable!(flag_name) + yield + ensure + if old.nil? + Current.feature_overrides&.delete(flag_name.to_sym) + else + Current.feature_overrides[flag_name.to_sym] = old + end + end + + private + + def feature_config(flag_name) + Rails.application.credentials.dig(:features, flag_name.to_sym) + end + + def user_allowed?(config, user) + return false if user.nil? + + allowed_emails = config[:allowed_emails] + return false if allowed_emails.blank? + + allowed_emails.include?(user.email) + end + + def cast_boolean(value) + ActiveModel::Type::Boolean.new.cast(value) || false + end +end diff --git a/config/credentials.yml.enc b/config/credentials.yml.enc new file mode 100644 index 00000000..62a84476 --- /dev/null +++ b/config/credentials.yml.enc @@ -0,0 +1 @@ +yjspzEXQf5fPuCTqkmnahCuQrqf3YDn0Tk1qyro/FP2Tp/Wz9P0CGt2+kkRBpl5ozR+vf6zLODRaFThjJgOazYqS6bWApMzo1jUn6kTPLTY9AOUF4VOxUajbZpwXUDJL/nG0sdg3y+dv3g6/nyCgu6BNndO1BJBezgFRoocUFfKyAQXkZTs7c0KCIsLjoJlK8pY+l6d5jowQqmaB6gJVTCBXDAV5Jr3n/wSM2gzQuVEup22em/sbRCXXR2GPdHgfXtthEMYh5ePOwFGqFimDymO4ZqrnTNHzEiO1zJdeAjdGjg0mNarFbefMtaLpKbnxeqaObm2tOPQVhEIGXOhIsB+a5oLJjnXuDSe5+UoYIBXHygRxx8p6ktqWovyIqDLWB8ojDW/YwrqXy4qoj8SACvg4tKQcKRhFJGJwXxzN2bu3G5yZVZQ75uvSwf0FEOATBEMqOnlvjzPZsvEKQnPCxqCPMf54R8YnNVEbdv+DWtWJlmSJrA6p+/jQ7ZgXmgyIWl/merILgSaCXcbMPqCoVwl1wLkdFjab5d/e6/xz2i/giPHv31o1bvl8RGFhZOz2DeDnqiYMv3GZ/f3oDeiMv/8LamSkQYPr3DqtFmms1UA=--oKBd19vdWhjbzt0B--ppPQiT/cJjaOxgDtNZyU1A== \ No newline at end of file diff --git a/spec/models/feature_flags_spec.rb b/spec/models/feature_flags_spec.rb new file mode 100644 index 00000000..740d060c --- /dev/null +++ b/spec/models/feature_flags_spec.rb @@ -0,0 +1,149 @@ +require "rails_helper" + +RSpec.describe FeatureFlags do + after { Current.feature_overrides = nil } + + describe ".enabled?" do + context "with request-scoped override" do + it "returns true when flag is overridden to true" do + described_class.enable!(:beacons) + + expect(described_class.enabled?(:beacons)).to be true + end + + it "returns false when flag is overridden to false" do + described_class.disable!(:beacons) + + expect(described_class.enabled?(:beacons)).to be false + end + + it "takes precedence over credentials" do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :beacons).and_return(true) + + described_class.disable!(:beacons) + + expect(described_class.enabled?(:beacons)).to be false + end + end + + context "with ENV override" do + it "returns true when ENV is set to true" do + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with("FEATURE_BEACONS").and_return("true") + + expect(described_class.enabled?(:beacons)).to be true + end + + it "returns false when ENV is set to false" do + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with("FEATURE_BEACONS").and_return("false") + + expect(described_class.enabled?(:beacons)).to be false + end + end + + context "with boolean credential" do + it "returns true when credential is true" do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :my_flag).and_return(true) + + expect(described_class.enabled?(:my_flag)).to be true + end + + it "returns false when credential is false" do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :my_flag).and_return(false) + + expect(described_class.enabled?(:my_flag)).to be false + end + end + + context "with hash credential and allowed_emails" do + before do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :beacons) + .and_return({ enabled: false, allowed_emails: [ "admin@skillrx.org" ] }) + end + + it "returns true for a user whose email is allowed" do + user = build(:user, email: "admin@skillrx.org") + + expect(described_class.enabled?(:beacons, user: user)).to be true + end + + it "returns false for a user whose email is not allowed" do + user = build(:user, email: "other@example.com") + + expect(described_class.enabled?(:beacons, user: user)).to be false + end + + it "returns false when no user is provided" do + expect(described_class.enabled?(:beacons)).to be false + end + end + + context "with globally enabled hash credential" do + before do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :beacons) + .and_return({ enabled: true, allowed_emails: [ "admin@skillrx.org" ] }) + end + + it "returns true regardless of user" do + expect(described_class.enabled?(:beacons)).to be true + end + end + + context "with unknown flag" do + it "returns false" do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :unknown).and_return(nil) + + expect(described_class.enabled?(:unknown)).to be false + end + end + end + + describe ".disabled?" do + it "returns the inverse of enabled?" do + allow(Rails.application.credentials).to receive(:dig) + .with(:features, :beacons).and_return(true) + + expect(described_class.disabled?(:beacons)).to be false + end + end + + describe ".with" do + it "enables the flag within the block" do + value_inside = nil + + described_class.with(:beacons, true) do + value_inside = described_class.enabled?(:beacons) + end + + expect(value_inside).to be true + expect(described_class.enabled?(:beacons)).to be false + end + + it "disables the flag within the block" do + described_class.enable!(:beacons) + + value_inside = nil + described_class.with(:beacons, false) do + value_inside = described_class.enabled?(:beacons) + end + + expect(value_inside).to be false + expect(described_class.enabled?(:beacons)).to be true + end + + it "restores the previous override after the block" do + described_class.enable!(:beacons) + + described_class.with(:beacons, false) { } + + expect(Current.feature_overrides[:beacons]).to be true + end + end +end