Migrate and delete ClientApplication Sequel model #15886
Conversation
|
|
||
| def reset_client_application! | ||
| client_application&.destroy | ||
| Carto::ClientApplication.create(user_id: id) |
There was a problem hiding this comment.
I believe the Sequel equivalent of this method raised an exception when it failed:
def reset_client_application!
if client_application
client_application.destroy
end
ClientApplication.create(:user_id => self.id)
endCould you use Carto::ClientApplication.create! instead to maintain the previous behavior?
| @@ -51,7 +51,7 @@ class Carto::User < ActiveRecord::Base | |||
| has_many :permissions, inverse_of: :owner, foreign_key: :owner_id | |||
| has_many :connector_configurations, inverse_of: :user, dependent: :destroy | |||
|
|
|||
| has_many :client_applications, class_name: Carto::ClientApplication, dependent: :destroy | |||
| has_one :client_application, class_name: Carto::ClientApplication, dependent: :destroy | |||
app/models/user.rb
Outdated
| @@ -437,7 +436,7 @@ def before_destroy(skip_table_drop: false) | |||
|
|
|||
| assign_search_tweets_to_organization_owner | |||
|
|
|||
| ClientApplication.where(user_id: id).each(&:destroy) | |||
| Carto::ClientApplication.where(user_id: id).each(&:destroy) | |||
There was a problem hiding this comment.
ActiveRecord already has a method which takes care of calling destroy on each of the association records:
Carto::ClientApplication.where(user_id: id).destroy_allIf you deem appropiate you can replace it.
| def build_client_applications_from_hash(client_app_hash) | ||
| return [] unless client_app_hash | ||
| def build_client_application_from_hash(client_app_hash) | ||
| return nil unless client_app_hash |
There was a problem hiding this comment.
No big deal but I think this is equivalent to:
return unless client_app_hash| [client_application] | ||
| # Overwrite fields that were created with ORM lifecycle callbacks | ||
| client_application.key = client_app_hash[:key] | ||
| client_application.secret = client_app_hash[:secret] |
There was a problem hiding this comment.
Another way of achieving this could be to modify the model callbacks so those attributes are only assigned if not present yet, for example:
def initialize_entity
self.key = OAuth::Helper.generate_key(40)[0, 40] unless key.present?
self.secret = OAuth::Helper.generate_key(40)[0, 40] unless secret.present?
endI don't have a strong opinion on this so choose what you think it's more intuitive to understand.
Maybe created_at is not overwritten by Rails if you pass it as an argument to the model constructor, but for updated_at you'll have to force it anyways.
| @@ -172,7 +172,7 @@ def destroy_user(user = @user) | |||
| st.destroy | |||
| end | |||
| end | |||
| ClientApplication.where(user_id: @user.id).each(&:destroy) | |||
| Carto::ClientApplication.where(user_id: @user.id).each(&:destroy) | |||
There was a problem hiding this comment.
I think that now that you've fixed the 1-1 relationship, this could be rewritten as:
user.client_application.destroy| super.sub('carto.com', 'cartodb.com') | ||
| end | ||
|
|
||
| end |
There was a problem hiding this comment.
Ruby and Rails have a set of rules about how directories/classes should be named in order for the autoloader to work more seamlessly, and avoid to manually having to write explicitly things such as require_relative or require_dependency.
I know this code is just a port from the previous model, but if you feel like so you could move the DomainPatcherRequestProxy class to its own file in order to follow the conventions.
I this case i'd move it to lib/domain_patcher_request_proxy.rb since it's not per se a model, and you'll have to manually require it anyways since lib contents are not loaded in Rails by default.
For more info you can check the Zeitwerk documentation, which is the autoloader of Rails 6 (but the conventions exists from earlier).
There was a problem hiding this comment.
Totally agree: having several classes in the same file is totally agains any kind of autoloader (and not only for Ruby)
thedae
deleted the
chore/ch106931/migrate-and-delete-clientapplication-model
branch
The spec tests are passing, but there are some problems with the Travis checks that are unrelated to these changes. Summoning @Jesus89 , he might have more information about this issue.