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 a join model for request issues and decision issues #7886

Merged
merged 4 commits into from
Nov 16, 2018

Conversation

anyakhvost
Copy link

This is the first PR for #7884

Description

  1. Create a join model RequestDecisionIssue and the associated table.
  2. Start writing to the new table for HLR and SC as well as to the source_request_issue_id column

@ghost ghost assigned anyakhvost Nov 16, 2018
@ghost ghost added the In-Progress label Nov 16, 2018
Copy link
Contributor

@pkarman pkarman left a comment

Choose a reason for hiding this comment

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

lgtm - only actionable thing is we probably want to wrap all the db inserts in a transaction.

@@ -47,14 +47,20 @@ def save_decision_issue
# if a DecisionIssue already exists then do not touch it. These should be immutable.
return if decision_issue

DecisionIssue.create!(
created_decision_issue = DecisionIssue.create!(
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want to wrap the 2 create calls in a transaction

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 sir!

create_table :request_decision_issues do |t|
t.integer :request_issue_id
t.integer :decision_issue_id
t.timestamps
Copy link
Contributor

Choose a reason for hiding this comment

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

just an observation that we haven't consistently used the AR timestamps on our models. I like doing it, and maybe we should formally endorse that pattern system-wide.

Copy link
Author

Choose a reason for hiding this comment

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

I always add these. I think they are useful.

@anyakhvost anyakhvost added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Nov 16, 2018
@ghost ghost assigned va-bot Nov 16, 2018
@va-bot va-bot merged commit f750a14 into master Nov 16, 2018
@va-bot va-bot deleted the anya/7884_many_to_many_request_decision_issues branch November 16, 2018 18:55
@ghost ghost removed the In-Progress label Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants