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

Fixes #29482 - favor update over update_attributes #497

Merged
merged 1 commit into from Apr 17, 2020

Conversation

ezr-ondrej
Copy link
Member

update_attributes got deprecated in Rails 6.0

@theforeman-bot
Copy link
Member

Issues: #29482

@ezr-ondrej
Copy link
Member Author

Let's wait for #498 and fix theese by rubocop.

@mmoll
Copy link
Contributor

mmoll commented Apr 17, 2020

[test foreman_discovery]

@mmoll
Copy link
Contributor

mmoll commented Apr 17, 2020

Fixes at least the update_attributes related Rails 6 test fialures, so I'd vote to merge this merge now and look into the remaining problems.

@lzap
Copy link
Member

lzap commented Apr 17, 2020

Isn't this a semantic change?

https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update_attribute

This is the fastest way to update attributes because it goes straight to the database, but take into account that in consequence the regular update procedures are totally bypassed. In particular: Validations are skipped. Callbacks are skipped.

But.

https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update

Updates the attributes of the model from the passed-in hash and saves the record, all wrapped in a transaction. If the object is invalid, the saving will fail and false will be returned.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Ondra explained to me that I actually thought update_columns. Yeah, let's unblock this.

@mmoll
Copy link
Contributor

mmoll commented Apr 17, 2020

@lzap it's update_attributes with an s at the end ;)

@lzap
Copy link
Member

lzap commented Apr 17, 2020

For the record, these guys are failing:

    OrgAdminJSTest::user is org admin of two organizations through single role.test_org_admins_of_two_orgs_can_work_with_resources_in_their_organizations
    Api::V2::DiscoveryRulesControllerTest.test_0001_should get index
    Api::V2::DiscoveryRulesControllerTest.test_0002_should search discovery rule
    Api::V2::DiscoveryRulesControllerTest.test_0003_should show discovery rule with taxonomy

@lzap lzap merged commit 4168e05 into theforeman:develop Apr 17, 2020
@lzap
Copy link
Member

lzap commented Apr 17, 2020

I can fix the discovery rules easily:

Failure:
Api::V2::DiscoveryRulesControllerTest#test_0003_should show discovery rule with taxonomy [/home/lzap/work/foreman_discovery/test/functional/api/v2/discovery_rules_controller_test.rb:28]:
Expected response to be a <2XX: success>, but was a <500: Internal Server Error>
Response body: {
  "error": {"message":"undefined local variable or method `implicit_order_column' for #<ActiveRecord::Associations::CollectionProxy []>"}
}

Not sure about that JS one, I don't see it locally.

@mmoll
Copy link
Contributor

mmoll commented Apr 17, 2020

@lzap The JS test was transient. Rails 5 is green and Rails 6 failing with the three errors you listed above: https://ci.theforeman.org/job/test_plugin_matrix/1950/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants