-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adds variant details to split_details endpoint #45
Adds variant details to split_details endpoint #45
Conversation
Needs somebody from @Betterment/test_track_core to claim domain review. Use the shovel operator to claim, e.g.:
|
text-transform: capitalize; | ||
} | ||
.DescriptionTable { | ||
@include standard-table(100%, 15%, 35%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new table style to support the split show page layout
@@ -1 +1,2 @@ | |||
json.(@split_detail, :name, :hypothesis, :assignment_criteria, :description, :owner, :location, :platform) | |||
json.variant_details @split_detail.split.variant_details, partial: 'variant_detail', as: :variant_detail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I delegate variant_details
to split
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do that. you'll also want to add includes(:variant_details)
wherever the split is being queried
nanda? |
Needs somebody from @Betterment/test_track_core and @dschaub to claim domain review. Use the shovel operator to claim, e.g.:
|
<< domain tafn |
@agirlnamedsophia needs to incorporate feedback from @dschaub. Bump when done. |
bump @dschaub |
Needs @dschaub to provide domain review. When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:
If you're too busy to review, unclaim the PR, e.g.:
|
end | ||
|
||
def update | ||
@split_detail = Split.find(params[:split_id]).detail | ||
@split_detail = Split.includes(:variant_details).find(params[:split_id]).detail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, I read the view wrong and thought we were iterating over the splits and causing an n+1, the way this worked was totally fine since it's a single query per table.
domain lgtm |
Ready to merge! :squirrel: 🎈 🤘 |
/domain @Betterment/test_track_core @dschaub