Edit EP Claim Labels | Add End Product Update model

[Sjones352]

Connects #15330

Description

This pr adds a new End Product Update table and model

Acceptance Criteria

• DB migration to add EndProductUpdate
  • Timestamps
  • User ID
  • EndProductEstablishment ID
  • Original code
  • New code
  • Original decision review ID
  • Status (success/error)
  • Error
  • IDs of active request issues on the end product establishment

Database Changes

Only for Schema Changes

• Timestamps (created_at, updated_at) for new tables
• Column comments updated
• Have your migration classes inherit from Caseflow::Migration, especially when adding indexes (use add_safe_index)
• Verify that migrate:rollback works as desired (change supported functions)
• Query profiling performed (eyeball Rails log, check bullet and fasterer output)
• Appropriate indexes added (especially for foreign keys, polymorphic columns, unique constraints, and Rails scopes)
• DB schema docs updated with make docs (after running make migrate)
• #appeals-schema notified with summary and link to this PR

[va-bot]

	2 Warnings
⚠️	This PR changes the schema. Please use the PR template checklist.
⚠️	This PR contains DB migrations that use add_index. Prefer Caseflow::Migration with add_safe_index instead. See https://github.com/department-of-veterans-affairs/caseflow/wiki/Writing-DB-migrations#index-creation-should-go-in-its-own-migration-file

Generated by 🚫 Danger

[codeclimate]

Code Climate has analyzed commit f10fb43 and detected 0 issues on this pull request.

View more on Code Climate.

[leikkisa]

Added some comments on your comments ;-)

[nanotone]

Bigger question: wouldn't we want EPU to be an Asyncable? Similar to all the other objects that track VBMS operations which might fail and need retries. In which case, the table would need a handful of Asyncable fields.

[yoomlam]

Some wording suggestions

[leikkisa]

Adding some responses to other comments, and I also updated the AC on the original ticket to reflect comments, and also be specific about which values are nullable (let's put a null: false on the other ones).

I think it's also a good idea to add an index by updated_at.

[nanotone]

This is getting close! Added a couple more suggestions.

[yoomlam]

I like how this article includes tests to ensure their polymorphic association migration works as desired.

[yoomlam]

Did some playing around and came up with this (posting for future reference; commented out code shows the code I was able to remove):

class CreateEndProductUpdate < Caseflow::Migration
  disable_ddl_transaction!  # See Note 2 below
  def change
    create_table :end_product_updates, comment: "Updates the claim label for end products established from Caseflow" do |t|
      # t.integer :user_id, comment: "The ID of the user who makes an end product update."
      # t.bigint :end_product_establishment_id, null: false, comment: "The end product establishment id used to track the end product being updated"
      # t.bigint :original_decision_review_id, null: false, comment: "The original ID of the decision review that this end product update belongs to; has a non-nil value only if a new decision_review was created."
      # t.string :original_decision_review_type, null: false, comment: "The original decision review type that this end product update belongs to"
      t.string :status, null: false, comment: "Status after an attempt to update the end product; expected values: 'success', 'error', ..."
      t.string :error, null: false, comment: "The error message captured from BGS if the end product update failed."
      t.string :original_code, comment: "The original end product code before the update was submitted"
      t.string :new_code, comment: "The new end product code the user wants to update to."
      t.integer :active_request_issue_ids, null: false, array: true, default: [], comment: "A list of active request issue IDs when a user has finished editing a decision review. Used to keep track of which request issues may have been impacted by the update."

      t.timestamps null: false

      t.references :user, null: false, foreign_key: true, comment: "The ID of the user who makes an end product update."
      t.references :end_product_establishment, null: false, foreign_key: true, comment: "The end product establishment id used to track the end product being updated"
      # t.index ["user_id"], name: "index_end_product_updates_on_user_id"
      # t.index ["end_product_establishment_id"], name: "index_end_product_updates_on_end_product_establishment_id"

      # Note 1: polymorphic reference cannot have a foreign_key
      # Note 1b: if this wasn't polymorphic you can use `foreign_key: {to_table: :appeals}` -- https://stackoverflow.com/questions/2933582/rails-migration-t-references-with-alternative-name
      # Note 2: strong_migrations warns to create the index separately (below), so set 'index: false' here
      t.references :original_decision_review, null: true, polymorphic: true, index: false, comment: "The original decision review that this end product update belongs to; has a non-nil value only if a new decision_review was created."
    end

    # Adding index separately as strong_migrations suggests
    add_index :end_product_updates, [:original_decision_review_type, :original_decision_review_id], algorithm: :concurrently, name: "index_epupdates_on_decision_review_type_and_decision_review_id"

    # Could not set the comment when calling `t.references` above, so doing it here
    change_column_comment :end_product_updates, :original_decision_review_type, "The original decision review type that this end product update belongs to"

    # Not needed due to `t.references` above
    # add_foreign_key "end_product_updates", "users", column: "user_id"
    # add_foreign_key "end_product_updates", "end_product_establishments", column: "end_product_establishment_id"
  end
end

Benefits:

• creates columns with correct types (e.g., bigint user_id)
• automatically add indices and add_foreign_key

[nanotone]

@yoomlam In your suggested t.references :original_decision_review line, what does the references: :decision_review argument do? As far as I understand, it should have no effect for polymorphic references.

[yoomlam]

@yoomlam In your suggested t.references :original_decision_review line, what does the references: :decision_review argument do? As far as I understand, it should have no effect for polymorphic references.

It points to the table that original_decision_review is referring to. Leaving it out doesn't seem to change the resulting schema.rb, but it's nice to have that reference be explicit since the column has a different name.

[nanotone]

It points to the table that original_decision_review is referring to. Leaving it out doesn't seem to change the resulting schema.rb, but it's nice to have that reference be explicit since the column has a different name.

Yeah, that's my point: there is no original decision_reviews table, since it's polymorphic.

[yoomlam]

It points to the table that original_decision_review is referring to. Leaving it out doesn't seem to change the resulting schema.rb, but it's nice to have that reference be explicit since the column has a different name.

Yeah, that's my point: there is no original decision_reviews table, since it's polymorphic.

Ah, right. Updated the code in my comment.

[nanotone]

Looks ready for to me!

[yoomlam]

Nice!