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.

Sign up for GitHub

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

Jump to bottom

Create model definitions and user associations for #20426

Merged
merged 37 commits into from
Jan 29, 2025
Merged

Create model definitions and user associations for #20426

merged 37 commits into from
Jan 29, 2025

Conversation

anjolovic
Copy link
Contributor

Summary

  • This work is behind a feature toggle (flipper): NO
  • Enhanced test coverage for the UserAction model by adding comprehensive tests for factory traits and status transitions. Fixed incorrect validation testing approach for invalid enum values.
  • No bug fixes, this is a test enhancement PR
  • Solution improves test coverage and correctness by properly testing all status transitions and factory traits, while fixing an incorrect assumption about Rails enum validation behavior
  • Platform team, we own the maintenance of the user actions component

Related issue(s)

Testing done

  • New code is covered by unit tests
  • Old behavior: Tests were missing coverage for factory traits and status transitions. Invalid status test was incorrectly expecting validation errors instead of ArgumentError
  • Steps to verify changes:
    1. Run bundle exec rspec spec/models/user_action_spec.rb
    2. Verify all 18 examples pass
    3. Verify new tests cover:
      • Factory traits (:success_status, :error_status)
      • Status transitions (initial → success, initial → error)
      • Proper enum validation behavior
  • Not behind a flipper, no toggle-specific testing needed

What areas of the site does it impact?

This change only impacts the test suite for the UserAction model. No production code was modified, only test improvements were made.

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature
  • No error nor warning in the console (except Rails 8.0 enum deprecation warning, which is pre-existing)
  • Events are being sent to the appropriate logging solution (N/A - test only changes)
  • Documentation has been updated (N/A - no documentation changes needed for test updates)
  • No sensitive information is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (N/A - test only changes)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected (N/A - test only changes)
  • I added a screenshot of the developed feature (N/A - test only changes)

Requested Feedback

Please review the approach taken for testing Rails enums. We've updated the invalid status test to expect an ArgumentError instead of a validation error, as this better reflects Rails' actual behavior with enums. Let me know if you think we should handle this differently.

@anjolovic anjolovic requested a review from a team as a code owner January 23, 2025 21:04
@anjolovic anjolovic force-pushed the VI-863-create-model-definitions-and-user-associations branch from abcd51c to 32ed17a Compare January 23, 2025 21:08
@anjolovic anjolovic force-pushed the VI-863-create-model-definitions-and-user-associations branch from 30d28e0 to 32ed17a Compare January 23, 2025 21:10
@github-actions GitHub Actions
Copy link

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: app/models/user_action.rb

@anjolovic anjolovic requested review from bosawt and bramleyjl January 23, 2025 21:16
@github-actions GitHub Actions
Copy link

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: spec/factories/user_action_events.rb

@anjolovic anjolovic force-pushed the VI-863-create-model-definitions-and-user-associations branch from b3a1f1b to caf2119 Compare January 23, 2025 21:23
@github-actions GitHub Actions
Copy link

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: spec/factories/user_action_events.rb

@github-actions GitHub Actions
Copy link

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: spec/factories/user_action_events.rb

@github-actions GitHub Actions
Copy link

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: spec/factories/user_action_events.rb

@anjolovic anjolovic requested a review from bosawt January 28, 2025 21:33
@github-actions GitHub Actions
Copy link

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: spec/factories/user_action_events.rb

bosawt
bosawt reviewed Jan 28, 2025
View reviewed changes
.github/CODEOWNERS Outdated
@@ -1268,6 +1270,8 @@ spec/factories/supporting_evidence_attachment.rb @department-of-veterans-affairs
spec/factories/triage_teams.rb @department-of-veterans-affairs/vfs-mhv-secure-messaging @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/factories/user_acceptable_verified_credentials.rb @department-of-veterans-affairs/octo-identity
spec/factories/user_accounts.rb @department-of-veterans-affairs/octo-identity
spec/factories/user_action_event.rb @department-of-veterans-affairs/octo-identity
Copy link
Contributor

Choose a reason for hiding this comment

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

pluralize these:

  • user_action_events.rb
  • user_actions.rb

@anjolovic
Simplifies user action specs with focused test cases
a5a3649 
Removes unnecessary status predicate tests
Improves status enum testing with invalid status case
@anjolovic
Renames spec factory files to match plural convention:
d2a83ab 
- user_action_event.rb -> user_action_events.rb
- user_action.rb -> user_actions.rb
@anjolovic anjolovic requested a review from bosawt January 28, 2025 22:26
@va-vfs-bot va-vfs-bot temporarily deployed to VI-863-create-model-definitions-and-user-associations/main/main January 28, 2025 23:07 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to VI-863-create-model-definitions-and-user-associations/main/main January 29, 2025 01:57 Inactive
@platform-support-slack-integration platform-support-slack-integration bot mentioned this pull request Jan 29, 2025
Closed
@anjolovic anjolovic enabled auto-merge (squash) January 29, 2025 22:22
@stevenjcumming stevenjcumming merged commit 4950713 into master Jan 29, 2025
21 checks passed
@stevenjcumming stevenjcumming deleted the VI-863-create-model-definitions-and-user-associations branch January 29, 2025 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenjcumming stevenjcumming stevenjcumming approved these changes

@bosawt bosawt bosawt approved these changes

@rileyanderson rileyanderson rileyanderson approved these changes

@bramleyjl bramleyjl Awaiting requested review from bramleyjl

Assignees
No one assigned
Labels
ready-for-backend-review require-backend-approval test-passing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@anjolovic @rileyanderson @bramleyjl @bosawt @stevenjcumming @va-vfs-bot