Skip to content
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

Create user_actions and user_action_events tables #20349

Closed

Conversation

anjolovic
Copy link

Summary

  • This work is behind a feature toggle (flipper): NO
  • Creating two new database tables (user_actions and user_action_events) to support user action auditing
  • Identity team owns and will maintain these components
  • This is foundational database work for the user action auditing feature

Related issue(s)

Testing done

  • New code is covered by unit tests
  • This is a new feature, adding database tables for user action auditing
  • Steps to verify:
    1. Run migrations locally: bin/rails db:drop db:create db:schema:load db:migrate
    2. Verify table structure in database matches requirements
    3. Test enum type constraints work for status values
    4. Verify foreign key relationships between tables
    5. Verify boolean constraints prevent three-state values
    6. Confirm indexes are properly created on status field

What areas of the site does it impact?

  • Database schema changes only
  • No direct user-facing impact yet
  • Foundation for future user action auditing features

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable)
  • No error nor warning in the console
  • No sensitive information is captured in logging, hardcoded, or specs
  • Documentation has been updated (migrations are self-documenting with comments)
  • Schema changes are clean with no unrelated tables or changes included

Database Changes

  • Created user_actions table with:
    • Core fields: uuid (as primary key), acting_user_account_id, subject_user_account_id, user_action_event_id
    • Status using PostgreSQL native enum type (initial, success, error) with built-in index
    • Additional columns: user_verified (boolean, not null), ip_address, device_info
    • Timestamps (created_at, updated_at)
    • All required fields have null: false constraint
  • Created user_action_events table with:
    • Fields: id (auto-generated), details (not null)
    • Timestamps (created_at, updated_at)
  • Added foreign key relationships following strong_migrations best practices:
    • user_actions.acting_user_account_id -> user_accounts.id
    • user_actions.subject_user_account_id -> user_accounts.id
    • user_actions.user_action_event_id -> user_action_events.id
    • All foreign keys are validated in a separate migration

Implementation Details

  1. Table Structure:

    • Follows the provided diagram exactly
    • Includes additional fields from requirements
    • Uses proper data types for each column
  2. Constraints and Validations:

    • Boolean fields prevent three-state values with null: false
    • Foreign keys are properly validated
    • Status field uses native PostgreSQL enum
  3. Indexing:

    • Status field indexed directly on enum definition
    • Foreign key fields automatically indexed

Requested Feedback

Please review:

  1. Table structure alignment with diagram
  2. Additional columns implementation
  3. PostgreSQL native enum implementation for status
  4. Foreign key implementation following strong_migrations pattern
  5. Index implementation directly on enum field
  6. Null constraints on required fields

Creates schema for tracking user-initiated actions with:
- User action events table for event details
- User actions table with:
  - PostgreSQL native enum for status (initial/success/error)
  - Foreign keys to user_accounts and user_action_events
  - Device info and IP address tracking
  - User verification status (boolean, not null)
  - UUID primary key

Follows strong_migrations best practices:
- Foreign keys added initially without validation
- Separate migration for foreign key validation
- Prevents write blocking during deployment

All required fields have null: false constraints
Indexes added directly to enum fields
@anjolovic anjolovic requested review from a team as code owners January 17, 2025 18:05
@va-vfs-bot va-vfs-bot temporarily deployed to VI-863-create-user-actions-and-events-tables-clean/main/main January 17, 2025 18:31 Inactive
validate_foreign_key :user_actions, :user_accounts, column: :subject_user_account_id
validate_foreign_key :user_actions, :user_action_events
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is missing an end of file new line

Copy link
Author

@anjolovic anjolovic Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's fixed now. Also, I am going to add this to RuboCop to always check and enforce newlines before commits.

   Layout/TrailingEmptyLines:
     Enabled: true
     EnforcedStyle: final_newline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could see if your text editor has a setting. In vscode it's:
Screenshot 2025-01-17 at 1 41 49 PM
This will ensure all the files you commit, not just ruby, have a new line

@va-vfs-bot va-vfs-bot temporarily deployed to VI-863-create-user-actions-and-events-tables-clean/main/main January 17, 2025 20:10 Inactive
Comment on lines 8 to 9
t.uuid :acting_user_account_id, null: false
t.uuid :subject_user_account_id, null: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use references here and then remove the add_foreign_keys at the end. This will make strong migrations happy and then we won't need the extra migrations to validate an empty table. This will also add an index to these columns, which we'll need.

t.references :acting_user_account, null: false, foreign_key: { to_table: :user_accounts }, type: :uuid
t.references :subject_user_account, null: false, foreign_key: { to_table: :user_accounts }, type: :uuid

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've updated to use references.

# Core fields
t.uuid :acting_user_account_id, null: false
t.uuid :subject_user_account_id, null: false
t.references :user_action_event, null: false, foreign_key: { validate: false }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to set foreign_key: { validate: false } since these are empty

t.references :user_action_event, null: false, foreign_key: true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Removed validate: false since we're creating empty tables. This aligns better with strong_migrations recommendations since there's no existing data to validate. Also removed the separate validation migration since it's no longer needed.

# Additional columns from ticket
t.boolean :user_verified, default: false, null: false
t.string :ip_address
t.jsonb :device_info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be a text column. I don't think we are going to parse the user agent

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed device_info from jsonb to text since we won't be parsing the user agent. Thanks for catching that!

…ate:false, change device_info type, remove separate validation migration)
@va-vfs-bot va-vfs-bot temporarily deployed to VI-863-create-user-actions-and-events-tables-clean/main/main January 17, 2025 21:16 Inactive
db/schema.rb Outdated
Comment on lines 1386 to 1397
create_table "user_actions", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
t.uuid "acting_user_account_id", null: false
t.uuid "subject_user_account_id", null: false
t.bigint "user_action_event_id", null: false
t.enum "status", default: "initial", null: false, enum_type: "user_action_status"
t.boolean "user_verified", default: false, null: false
t.string "ip_address"
t.jsonb "device_info"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["status"], name: "index_user_actions_on_status"
t.index ["user_action_event_id"], name: "index_user_actions_on_user_action_event_id"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the schema didn't get updated with the latest changes. Could you re-run the migrations?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Working on fixing it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's fixed now.

@anjolovic
Copy link
Author

anjolovic commented Jan 17, 2025

I am working on fixing the schema.rb to pick these changes.

@va-vfs-bot va-vfs-bot temporarily deployed to VI-863-create-user-actions-and-events-tables-clean/main/main January 17, 2025 22:09 Inactive
…emove validate:false, change to text columns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants