From 8700beea607f469b206a7cd0a7a9fe58a6143226 Mon Sep 17 00:00:00 2001 From: Priit Tark Date: Thu, 23 Jul 2015 17:44:52 +0300 Subject: [PATCH 1/4] Logging user actions #2808 --- app/models/contact.rb | 4 +-- app/models/domain.rb | 1 - app/models/epp/domain.rb | 2 +- .../initializers/eis_custom_active_model.rb | 18 ------------- .../initializers/eis_custom_active_record.rb | 7 ----- config/initializers/eis_custom_flash.rb | 27 ------------------- config/initializers/eis_custom_rack.rb | 14 ---------- 7 files changed, 3 insertions(+), 70 deletions(-) delete mode 100644 config/initializers/eis_custom_active_model.rb delete mode 100644 config/initializers/eis_custom_active_record.rb delete mode 100644 config/initializers/eis_custom_flash.rb delete mode 100644 config/initializers/eis_custom_rack.rb diff --git a/app/models/contact.rb b/app/models/contact.rb index 7db3e5ea6..b3d932bcc 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -311,8 +311,8 @@ class Contact < ActiveRecord::Base def status_notes_array=(notes) self.status_notes = {} notes ||= [] - statuses.each_with_index do |status,i| - self.status_notes[status] = notes[i] + statuses.each_with_index do |status, i| + status_notes[status] = notes[i] end end diff --git a/app/models/domain.rb b/app/models/domain.rb index 70eedccc8..89dffb5b9 100644 --- a/app/models/domain.rb +++ b/app/models/domain.rb @@ -78,7 +78,6 @@ class Domain < ActiveRecord::Base end after_save :update_whois_record - after_create :update_reserved_domains def update_reserved_domains return unless in_reserved_list? diff --git a/app/models/epp/domain.rb b/app/models/epp/domain.rb index 57cd1b5a3..b953e52ab 100644 --- a/app/models/epp/domain.rb +++ b/app/models/epp/domain.rb @@ -11,7 +11,7 @@ class Epp::Domain < Domain before_validation :validate_contacts def validate_contacts - return if contacts.map {|c| c.valid? }.all? + return if contacts.map(&:valid?).all? add_epp_error('2304', nil, nil, I18n.t(:object_status_prohibits_operation)) false end diff --git a/config/initializers/eis_custom_active_model.rb b/config/initializers/eis_custom_active_model.rb deleted file mode 100644 index f41a42325..000000000 --- a/config/initializers/eis_custom_active_model.rb +++ /dev/null @@ -1,18 +0,0 @@ -# Log all active model user errors -module ActiveModel - class Errors - def add(attribute, message = :invalid, options = {}) - message = normalize_message(attribute, message, options) - if exception = options[:strict] - exception = ActiveModel::StrictValidationFailed if exception == true - raise exception, full_message(attribute, message) - end - - # CUSTOM logging - Rails.logger.info "USER MSG: ACTIVEMODEL: #{@base.try(:class)} [#{attribute}] #{message}" if message.present? - # END of CUSTOM logging - - self[attribute] << message - end - end -end diff --git a/config/initializers/eis_custom_active_record.rb b/config/initializers/eis_custom_active_record.rb deleted file mode 100644 index 546f5a4ae..000000000 --- a/config/initializers/eis_custom_active_record.rb +++ /dev/null @@ -1,7 +0,0 @@ -# Log all user issues raised by active record -class ActiveRecord::Base - after_validation do |m| - Rails.logger.info "USER MSG: ACTIVERECORD: #{m.class} ##{m.id} #{m.errors.full_messages} #{m.errors['epp_errors']}" if m.errors.present? - true - end -end diff --git a/config/initializers/eis_custom_flash.rb b/config/initializers/eis_custom_flash.rb deleted file mode 100644 index ed5832e85..000000000 --- a/config/initializers/eis_custom_flash.rb +++ /dev/null @@ -1,27 +0,0 @@ -# Log all flash messages -module ActionDispatch - class Flash - def call(env) - @app.call(env) - ensure - session = Request::Session.find(env) || {} - flash_hash = env[KEY] - - if flash_hash && (flash_hash.present? || session.key?('flash')) - session["flash"] = flash_hash.to_session_value - - # EIS custom logging - Rails.logger.info "USER MSG: FLASH: #{session['flash']['flashes'].inspect}" if session['flash'] - # END OF EIS custom logging - - env[KEY] = flash_hash.dup - end - - if (!session.respond_to?(:loaded?) || session.loaded?) && # (reset_session uses {}, which doesn't implement #loaded?) - session.key?('flash') && session['flash'].nil? - session.delete('flash') - end - end - end -end - diff --git a/config/initializers/eis_custom_rack.rb b/config/initializers/eis_custom_rack.rb deleted file mode 100644 index 52dbd8244..000000000 --- a/config/initializers/eis_custom_rack.rb +++ /dev/null @@ -1,14 +0,0 @@ -# EIS custom rack hack in order to enable test external interfaces EPP/REPP inside webserver network -# rubocop:disable Metrics/LineLength -module Rack - class Request - def trusted_proxy?(ip) - if ENV['eis_trusted_proxies'] - ENV['eis_trusted_proxies'].split(',').map(&:strip).include?(ip) - else - ip =~ /\A127\.0\.0\.1\Z|\A(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\.|\A::1\Z|\Afd[0-9a-f]{2}:.+|\Alocalhost\Z|\Aunix\Z|\Aunix:/i - end - end - end -end -# rubocop:enable Metrics/LineLength From 48bc4d4ac98d66a51606a4f8807c3d366f0b68ad Mon Sep 17 00:00:00 2001 From: Priit Tark Date: Thu, 23 Jul 2015 20:02:01 +0300 Subject: [PATCH 2/4] Optimize contact linked status update #2477 --- app/models/contact.rb | 38 +++++++++++++++++++++++-------------- app/models/epp/domain.rb | 18 ++++++++++++++---- config/environments/test.rb | 11 ++++++++++- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index b3d932bcc..44af31d65 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -260,21 +260,11 @@ class Contact < ActiveRecord::Base Country.new(country_code) end - # Find a way to use self.domains with contact - def domains_owned - Domain.where(registrant_id: id) - end - - def relations_with_domain? - return true if domain_contacts.present? || domains_owned.present? - false - end - # TODO: refactor, it should not allow to destroy with normal destroy, # no need separate method # should use only in transaction def destroy_and_clean - if relations_with_domain? + if domains_present? errors.add(:domains, :exist) return false end @@ -316,14 +306,34 @@ class Contact < ActiveRecord::Base end end + # optimization under children loop, + # otherwise bullet will not be happy + def domains_present? + return @domains_present if @domains_present + domain_contacts.present? || registrant_domains.present? + end + + # for overwrite when doing children loop + def domains_present=(boolean) + @domains_present = boolean + end + def manage_linked - if domains.present? - statuses << LINKED if statuses.detect { |s| s == LINKED }.blank? + if domains_present? + set_linked else - statuses.delete_if { |s| s == LINKED } + unset_linked end end + def set_linked + statuses << LINKED if statuses.detect { |s| s == LINKED }.blank? + end + + def unset_linked + statuses.delete_if { |s| s == LINKED } + end + # rubocop:disable Metrics/CyclomaticComplexity def manage_ok return unset_ok unless valid? diff --git a/app/models/epp/domain.rb b/app/models/epp/domain.rb index b953e52ab..73c015904 100644 --- a/app/models/epp/domain.rb +++ b/app/models/epp/domain.rb @@ -16,10 +16,20 @@ class Epp::Domain < Domain false end - before_save :update_contact_status - def update_contact_status - contacts.each do |c| - next if c.linked? + before_save :link_contacts + def link_contacts + # Based on bullet report + unlinked_contacts = contacts.select { |c| !c.linked? } # speed up a bit + unlinked_contacts.each do |uc| + uc.domains_present = true # no need to fetch domains again + uc.save(validate: false) + end + end + + after_destroy :unlink_contacts + def unlink_contacts + contacts.each do |c| + c.domains_present = false c.save(validate: false) end end diff --git a/config/environments/test.rb b/config/environments/test.rb index ee1ae12b7..80f7b9064 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -44,18 +44,27 @@ Rails.application.configure do # The available log levels are: :debug, :info, :warn, :error, :fatal, and :unknown, # corresponding to the log level numbers from 0 up to 5 respectively - config.log_level = :info + config.log_level = :debug # for finding database optimization config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true + Bullet.rails_logger = true Bullet.raise = true # raise an error if n+1 query occurs Bullet.unused_eager_loading_enable = false # Currenty hard to fix, it is triggered by Epp::Domain.new_from_epp for create request Bullet.add_whitelist type: :n_plus_one_query, class_name: 'Contact', association: :registrar + + # when domain updates, then we need to update all contact linked status, + # somehow it triggers bullet counter cache for versions, + # there was no output indicating each version where fetched or counted + # thus needs more investigation + Bullet.add_whitelist type: :counter_cache, class_name: 'Contact', association: :versions end + + # config.logger = Logger.new(STDOUT) end # In this mode, any jobs you queue will be run in the same thread, synchronously From 7fa9fb3c6eeb06bed2ff15c956b2673ac2799be6 Mon Sep 17 00:00:00 2001 From: Priit Tark Date: Mon, 27 Jul 2015 15:12:34 +0300 Subject: [PATCH 3/4] Added backtrace reverse info --- app/controllers/epp_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/epp_controller.rb b/app/controllers/epp_controller.rb index 3fec7910b..f81a89ea6 100644 --- a/app/controllers/epp_controller.rb +++ b/app/controllers/epp_controller.rb @@ -43,7 +43,8 @@ class EppController < ApplicationController if Rails.env.test? || Rails.env.development? # rubocop:disable Rails/Output puts e.backtrace.reverse.join("\n") - puts "\nFROM-EPP-RESCUE: #{e.message}\n" + puts "\n BACKTRACE REVERSED!\n" + puts "\n FROM-EPP-RESCUE: #{e.message}\n\n\n" # rubocop:enable Rails/Output else logger.error "FROM-EPP-RESCUE: #{e.message}" From a9ba4dbfd201cd801a03ddf489bd2efabcd2b066 Mon Sep 17 00:00:00 2001 From: Priit Tark Date: Mon, 27 Jul 2015 15:59:23 +0300 Subject: [PATCH 4/4] Contact linked test update/refactor #2411 --- app/models/contact.rb | 8 +++----- app/models/epp/domain.rb | 2 +- app/views/admin/contacts/partials/_domains.haml | 2 +- spec/fabricators/domain_fabricator.rb | 2 +- spec/mailers/contact_mailer_spec.rb | 1 + spec/models/contact_spec.rb | 10 ++++++++-- spec/rails_helper.rb | 1 + 7 files changed, 16 insertions(+), 10 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index 44af31d65..d1ea2da63 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -51,6 +51,9 @@ class Contact < ActiveRecord::Base manage_ok end + # for overwrite when doing children loop + attr_writer :domains_present + scope :current_registrars, ->(id) { where(registrar_id: id) } BIC = 'bic' @@ -313,11 +316,6 @@ class Contact < ActiveRecord::Base domain_contacts.present? || registrant_domains.present? end - # for overwrite when doing children loop - def domains_present=(boolean) - @domains_present = boolean - end - def manage_linked if domains_present? set_linked diff --git a/app/models/epp/domain.rb b/app/models/epp/domain.rb index 73c015904..c36912d6b 100644 --- a/app/models/epp/domain.rb +++ b/app/models/epp/domain.rb @@ -528,7 +528,7 @@ class Epp::Domain < Domain return if registrant.registrar_id == registrar_id is_other_domains_contact = DomainContact.where('contact_id = ? AND domain_id != ?', registrant_id, id).count > 0 - if registrant.domains_owned.count > 1 || is_other_domains_contact + if registrant.registrant_domains.count > 1 || is_other_domains_contact oc = copy_and_transfer_contact(registrant_id, registrar_id) self.registrant_id = oc.id else diff --git a/app/views/admin/contacts/partials/_domains.haml b/app/views/admin/contacts/partials/_domains.haml index 142c06397..0c319127b 100644 --- a/app/views/admin/contacts/partials/_domains.haml +++ b/app/views/admin/contacts/partials/_domains.haml @@ -8,7 +8,7 @@ %th{class: 'col-xs-4'}= t(:registrar) %th{class: 'col-xs-4'}= t(:valid_to) %tbody - - @contact.domains_owned.each do |x| + - @contact.registrant_domains.each do |x| %tr %td= link_to(x.name, [:admin, x]) %td= link_to(x.registrar, [:admin, x.registrar]) diff --git a/spec/fabricators/domain_fabricator.rb b/spec/fabricators/domain_fabricator.rb index 80d8bf598..24f7a45f4 100644 --- a/spec/fabricators/domain_fabricator.rb +++ b/spec/fabricators/domain_fabricator.rb @@ -3,7 +3,7 @@ Fabricator(:domain) do valid_to Date.new(2014, 8, 7) period 1 period_unit 'y' - registrant(fabricator: :registrant) + registrant { Fabricate(:registrant) } nameservers(count: 3) admin_domain_contacts(count: 1) { Fabricate(:admin_domain_contact) } tech_domain_contacts(count: 1) { Fabricate(:tech_domain_contact) } diff --git a/spec/mailers/contact_mailer_spec.rb b/spec/mailers/contact_mailer_spec.rb index 9f825b380..fe4ccc84c 100644 --- a/spec/mailers/contact_mailer_spec.rb +++ b/spec/mailers/contact_mailer_spec.rb @@ -29,6 +29,7 @@ describe ContactMailer do before :all do @domain = Fabricate(:domain) @contact = @domain.registrant + @contact.reload # until figured out why registrant_domains not loaded @contact.deliver_emails = true @contact.email = 'test@example.org' # new email @mail = ContactMailer.email_updated(@contact) diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb index 721039f14..6cc3ee124 100644 --- a/spec/models/contact_spec.rb +++ b/spec/models/contact_spec.rb @@ -137,8 +137,8 @@ describe Contact do end end - it 'should not have relation' do - @contact.relations_with_domain?.should == false + it 'should not have relation with domains' do + @contact.domains_present?.should == false end it 'bic should be valid' do @@ -234,9 +234,15 @@ describe Contact do it 'should have related domain descriptions hash' do contact = @domain.registrant + contact.reload # somehow it registrant_domains are empty? contact.related_domain_descriptions.should == { "#{@domain.name}" => [:registrant] } end + it 'should have related domain descriptions hash when find directly' do + contact = @domain.registrant + Contact.find(contact.id).related_domain_descriptions.should == { "#{@domain.name}" => [:registrant] } + end + it 'should have related domain descriptions hash' do contact = @domain.contacts.first contact.related_domain_descriptions.should == { "#{@domain.name}" => [:admin] } diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 453d8fc13..c36859300 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -48,6 +48,7 @@ def create_settings Setting.client_side_status_editing_enabled = true + # speedup and easier to create fabrications @fixed_registrar = Registrar.find_by_name('fixed registrar') || Fabricate(:registrar, name: 'fixed registrar', code: 'FIXED')