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

do not destroy location relations with invalid location updates #1893

Merged
merged 1 commit into from
Aug 18, 2016

Conversation

camallen
Copy link
Contributor

@camallen camallen commented Aug 3, 2016

closes #1885 - avoid invalid location updates on the media resource. Make sure these validations pass before updating the relation to reflect the updated location resource.

Describe your change here.

Review checklist

  • First, the most important one: is this PR small enough that you can actually review it? Feel free to just reject a branch if the changes are hard to review due to the length of the diff.
  • If there are any migrations, will they the previous version of the app work correctly after they've been run (e.g. the don't remove columns still known about by ActiveRecord).
  • If anything changed with regards to the public API, are those changes also documented in the apiary.apib file?
  • Are all the changes covered by tests? Think about any possible edge cases that might be left untested.

end

it 'should create externally linked media resources' do
update_params[:subjects].merge!(locations: external_locs)

Choose a reason for hiding this comment

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

Use update_params[:subjects][:locations] = external_locs instead of update_params[:subjects].merge!(locations: external_locs).

@camallen camallen force-pushed the subject_update_locations branch from 9032b6d to 4ecb8a6 Compare August 3, 2016 20:50
if locations_update
new_locations = add_locations(locations, resource)
end
resource.save!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually - think i'll move this to a block for the locations update method.

@camallen camallen force-pushed the subject_update_locations branch from 4ecb8a6 to cde8f3a Compare August 4, 2016 12:13
subject.locations.build(location_params)
new_locations = if locations.blank?
nil
else

Choose a reason for hiding this comment

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

Align else with if.

@camallen camallen force-pushed the subject_update_locations branch 2 times, most recently from 4d1a22c to 0f6a8bd Compare August 4, 2016 12:22
subject.locations = add_locations(locations, subject)
subject.save!
super(update_params, id)
add_locations(locations, resource) do |subject|

Choose a reason for hiding this comment

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

Pass &:save! as an argument to add_locations instead of a block.

run the has_many update after the resource validation has occurred not before
@camallen camallen force-pushed the subject_update_locations branch from 0f6a8bd to 53cf8a9 Compare August 4, 2016 12:25
@camallen
Copy link
Contributor Author

camallen commented Aug 4, 2016

thank you hound for barking at me for a simpler solution...much better now.

@marten marten merged commit 2f1126c into zooniverse:master Aug 18, 2016
@camallen camallen deleted the subject_update_locations branch September 2, 2016 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Error: Validation failed: Locations is invalid" will reset subject locations to []
4 participants