From 1db594fc1c61fd47a2c4284a61c20d231311fb60 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Wed, 18 Nov 2020 16:29:18 +0200
Subject: [PATCH 1/7] NS bulk change: Add tests, don't use name_puny format

---
 app/models/registrar.rb                       |  2 +-
 .../valid_domains_for_ns_replacement.csv      |  2 ++
 .../registrar/replace_nameservers_test.rb     | 16 +++++++++
 .../bulk_change/nameserver_test.rb            | 33 +++++++++++++++++++
 4 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 test/fixtures/files/valid_domains_for_ns_replacement.csv

diff --git a/app/models/registrar.rb b/app/models/registrar.rb
index 040c7886b..e79cd87f2 100644
--- a/app/models/registrar.rb
+++ b/app/models/registrar.rb
@@ -146,7 +146,7 @@ class Registrar < ApplicationRecord
       domain_list = []
 
       nameservers.where(hostname: hostname).find_each do |original_nameserver|
-        next unless domains.include?(original_nameserver.domain.name_puny) || domains.empty?
+        next unless domains.include?(original_nameserver.domain.name) || domains.empty?
 
         new_nameserver = Nameserver.new
         new_nameserver.domain = original_nameserver.domain
diff --git a/test/fixtures/files/valid_domains_for_ns_replacement.csv b/test/fixtures/files/valid_domains_for_ns_replacement.csv
new file mode 100644
index 000000000..122301ca8
--- /dev/null
+++ b/test/fixtures/files/valid_domains_for_ns_replacement.csv
@@ -0,0 +1,2 @@
+domain_name
+shop.test
diff --git a/test/models/registrar/replace_nameservers_test.rb b/test/models/registrar/replace_nameservers_test.rb
index 5bcbb83d1..36071e3fa 100644
--- a/test/models/registrar/replace_nameservers_test.rb
+++ b/test/models/registrar/replace_nameservers_test.rb
@@ -20,4 +20,20 @@ class ReplaceNameserversTest < ActiveSupport::TestCase
 
     assert_equal([], result)
   end
+
+  def test_replace_nameserver_in_bulk_respects_domain_limit_scope
+    eligible_domain = domains(:shop)
+    unscoped_domain = domains(:airport)
+
+    new_attributes = { hostname: 'ns-updated1.bestnames.test', ipv4: '192.0.3.1',
+      ipv6: '2001:db8::2' }
+
+    result = @registrar.replace_nameservers('ns1.bestnames.test', new_attributes, domains: ['shop.test'])
+    assert_equal(["shop.test"], result)
+
+    unscoped_domain.reload
+    eligible_domain.reload
+    assert eligible_domain.nameservers.where(hostname: 'ns1.bestnames.test').empty?
+    assert unscoped_domain.nameservers.where(hostname: 'ns1.bestnames.test').any?
+  end
 end
diff --git a/test/system/registrar_area/bulk_change/nameserver_test.rb b/test/system/registrar_area/bulk_change/nameserver_test.rb
index 3d4b4dc70..38c2cb9e6 100644
--- a/test/system/registrar_area/bulk_change/nameserver_test.rb
+++ b/test/system/registrar_area/bulk_change/nameserver_test.rb
@@ -57,4 +57,37 @@ class RegistrarAreaNameserverBulkChangeTest < ApplicationSystemTestCase
     assert_field 'ipv4', with: 'ipv4'
     assert_field 'ipv6', with: 'ipv6'
   end
+
+  def test_replaces_nameservers_only_for_scoped_domains
+    request_body = { data: { type: 'nameserver',
+                              id: 'ns1.bestnames.test',
+                             domains: ['shop.test'],
+                             attributes: { hostname: 'new-ns.bestnames.test',
+                                           ipv4: %w[192.0.2.55 192.0.2.56],
+                                           ipv6: %w[2001:db8::55 2001:db8::56] } } }
+    request_stub = stub_request(:put, /registrar\/nameservers/).with(body: request_body,
+                                                                     headers: { 'Content-type' => Mime[:json] },
+                                                                     basic_auth: ['test_goodnames', 'testtest'])
+                   .to_return(body: { data: [{
+                     type: 'nameserver',
+                     id: 'new-ns.bestnames.test'}],
+                     affected_domains: ["shop.test"]}.to_json, status: 200)
+
+    visit registrar_domains_url
+    click_link 'Bulk change'
+    click_link 'Nameserver'
+
+    fill_in 'Old hostname', with: 'ns1.bestnames.test'
+    fill_in 'New hostname', with: 'new-ns.bestnames.test'
+    fill_in 'ipv4', with: "192.0.2.55\n192.0.2.56"
+    fill_in 'ipv6', with: "2001:db8::55\n2001:db8::56"
+    attach_file :puny_file, Rails.root.join('test', 'fixtures', 'files', 'valid_domains_for_ns_replacement.csv').to_s
+
+    click_on 'Replace nameserver'
+
+    assert_requested request_stub
+    assert_current_path registrar_domains_path
+    assert_text 'Nameserver have been successfully replaced'
+    assert_text 'Affected domains: shop.test'
+  end
 end

From 980259814958291022b892a9f1fa247e6cff8078 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Wed, 18 Nov 2020 16:33:20 +0200
Subject: [PATCH 2/7] NS bulk change: Update domain WHOIS afterwards

---
 app/models/registrar.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/models/registrar.rb b/app/models/registrar.rb
index e79cd87f2..3b5eb3502 100644
--- a/app/models/registrar.rb
+++ b/app/models/registrar.rb
@@ -158,6 +158,7 @@ class Registrar < ApplicationRecord
         original_nameserver.destroy!
       end
 
+      self.domains.where(name: domain_list).find_each(&:update_whois_record) if domain_list.any?
       domain_list.uniq.sort
     end
   end

From 12cdc868de1451090c4b1159b503b25fcad5b78e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Mon, 21 Dec 2020 14:55:33 +0200
Subject: [PATCH 3/7] Replace nameservers: turn all domain names downcase

---
 app/api/repp/nameservers_v1.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/api/repp/nameservers_v1.rb b/app/api/repp/nameservers_v1.rb
index 56a893880..df3bb64eb 100644
--- a/app/api/repp/nameservers_v1.rb
+++ b/app/api/repp/nameservers_v1.rb
@@ -29,7 +29,7 @@ module Repp
           ipv6: params[:data][:attributes][:ipv6],
         }
 
-        domains = params[:data][:domains] || []
+        domains = params[:data][:domains].map(&:downcase) || []
 
         begin
           affected_domains = current_user.registrar.replace_nameservers(hostname, new_attributes,

From 7d61eb07b098257b6339daffb36be1fe58442367 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Mon, 28 Dec 2020 16:30:51 +0200
Subject: [PATCH 4/7] Fix test

---
 test/system/registrar_area/bulk_change/nameserver_test.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/system/registrar_area/bulk_change/nameserver_test.rb b/test/system/registrar_area/bulk_change/nameserver_test.rb
index e97b2d1c3..4f63eb061 100644
--- a/test/system/registrar_area/bulk_change/nameserver_test.rb
+++ b/test/system/registrar_area/bulk_change/nameserver_test.rb
@@ -72,8 +72,8 @@ class RegistrarAreaNameserverBulkChangeTest < ApplicationSystemTestCase
                                                                      basic_auth: ['test_goodnames', 'testtest'])
                    .to_return(body: { data: {
                      type: 'nameserver',
-                     id: 'new-ns.bestnames.test'},
-                     affected_domains: ["shop.test"]}.to_json, status: 200)
+                     id: 'new-ns.bestnames.test',
+                     affected_domains: ["shop.test"]}}.to_json, status: 200)
 
     visit registrar_domains_url
     click_link 'Bulk change'

From 0e120d499266468cea8f0a9f881166d31a3f7686 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Wed, 30 Dec 2020 11:00:52 +0200
Subject: [PATCH 5/7] Process failed domains for bulk ns change

---
 .../registrar/nameservers_controller.rb        |  4 ++++
 .../v1/registrar/nameservers_controller.rb     |  7 ++++---
 app/models/registrar.rb                        | 18 ++++++++++++------
 config/locales/registrar/nameservers.en.yml    |  1 +
 .../bulk_change/nameserver_test.rb             |  6 ++++--
 5 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/app/controllers/registrar/nameservers_controller.rb b/app/controllers/registrar/nameservers_controller.rb
index 2a22476be..1ab0e335f 100644
--- a/app/controllers/registrar/nameservers_controller.rb
+++ b/app/controllers/registrar/nameservers_controller.rb
@@ -51,6 +51,10 @@ class Registrar
         notices = [t('.replaced')]
         notices << "#{t('.affected_domains')}: " \
                    "#{parsed_response[:data][:affected_domains].join(', ')}"
+        if parsed_response[:data][:skipped_domains]
+          notices << "#{t('.skipped_domains')}: " \
+          "#{parsed_response[:data][:skipped_domains].join(', ')}"
+        end
 
         flash[:notice] = notices.join(', ')
         redirect_to registrar_domains_url
diff --git a/app/controllers/repp/v1/registrar/nameservers_controller.rb b/app/controllers/repp/v1/registrar/nameservers_controller.rb
index 1df781ad7..a71f11759 100644
--- a/app/controllers/repp/v1/registrar/nameservers_controller.rb
+++ b/app/controllers/repp/v1/registrar/nameservers_controller.rb
@@ -5,12 +5,12 @@ module Repp
         before_action :verify_nameserver_existance, only: %i[update]
 
         def update
-          affected = current_user.registrar
+          affected, errored = current_user.registrar
                                  .replace_nameservers(hostname,
                                                       hostname_params[:data][:attributes],
                                                       domains: domains_from_params)
 
-          render_success(data: data_format_for_success(affected))
+          render_success(data: data_format_for_success(affected, errored))
         rescue ActiveRecord::RecordInvalid => e
           handle_errors(e.record)
         end
@@ -23,12 +23,13 @@ module Repp
           params[:data][:domains].map(&:downcase)
         end
 
-        def data_format_for_success(affected_domains)
+        def data_format_for_success(affected_domains, errored_domains)
           {
             type: 'nameserver',
             id: params[:data][:attributes][:hostname],
             attributes: params[:data][:attributes],
             affected_domains: affected_domains,
+            skipped_domains: errored_domains,
           }
         end
 
diff --git a/app/models/registrar.rb b/app/models/registrar.rb
index c1168a887..e038cdb16 100644
--- a/app/models/registrar.rb
+++ b/app/models/registrar.rb
@@ -145,22 +145,28 @@ class Registrar < ApplicationRecord
   def replace_nameservers(hostname, new_attributes, domains: [])
     transaction do
       domain_list = []
+      failed_list = []
 
-      nameservers.where(hostname: hostname).find_each do |original_nameserver|
-        next unless domains.include?(original_nameserver.domain.name) || domains.empty?
+      nameservers.where(hostname: hostname).find_each do |origin|
+        next unless domains.include?(origin.domain.name) || domains.empty?
+
+        if origin.domain.nameservers.where(hostname: new_attributes[:hostname]).any?
+          failed_list << origin.domain.name
+          next
+        end
 
         new_nameserver = Nameserver.new
-        new_nameserver.domain = original_nameserver.domain
+        new_nameserver.domain = origin.domain
         new_nameserver.attributes = new_attributes
         new_nameserver.save!
 
-        domain_list << original_nameserver.domain.name
+        domain_list << origin.domain.name
 
-        original_nameserver.destroy!
+        origin.destroy!
       end
 
       self.domains.where(name: domain_list).find_each(&:update_whois_record) if domain_list.any?
-      domain_list.uniq.sort
+      [domain_list.uniq.sort, failed_list.uniq.sort]
     end
   end
 
diff --git a/config/locales/registrar/nameservers.en.yml b/config/locales/registrar/nameservers.en.yml
index 6cc08f0ab..2e645734d 100644
--- a/config/locales/registrar/nameservers.en.yml
+++ b/config/locales/registrar/nameservers.en.yml
@@ -4,3 +4,4 @@ en:
       update:
         replaced: Nameserver have been successfully replaced
         affected_domains: Affected domains
+        skipped_domains: Skipped domains
diff --git a/test/system/registrar_area/bulk_change/nameserver_test.rb b/test/system/registrar_area/bulk_change/nameserver_test.rb
index 4f63eb061..0ba8f7ba2 100644
--- a/test/system/registrar_area/bulk_change/nameserver_test.rb
+++ b/test/system/registrar_area/bulk_change/nameserver_test.rb
@@ -18,7 +18,8 @@ class RegistrarAreaNameserverBulkChangeTest < ApplicationSystemTestCase
                      .to_return(body: { data: {
                                             type: 'nameserver',
                                             id: 'new-ns.bestnames.test',
-                                            affected_domains: ["airport.test", "shop.test"]
+                                            affected_domains: ["airport.test", "shop.test"],
+                                            skipped_domains: []
                                         }
                                       }.to_json, status: 200)
 
@@ -73,7 +74,8 @@ class RegistrarAreaNameserverBulkChangeTest < ApplicationSystemTestCase
                    .to_return(body: { data: {
                      type: 'nameserver',
                      id: 'new-ns.bestnames.test',
-                     affected_domains: ["shop.test"]}}.to_json, status: 200)
+                     affected_domains: ["shop.test"],
+                     skipped_domains: []}}.to_json, status: 200)
 
     visit registrar_domains_url
     click_link 'Bulk change'

From 8bb0fd80d1b6877a7b1816ea00c2f66783c92b2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Wed, 30 Dec 2020 11:09:46 +0200
Subject: [PATCH 6/7] Allow punycode domains in bulk ns change domain scope

---
 app/models/registrar.rb                           | 8 +++++---
 test/integration/api/nameservers/put_test.rb      | 3 ++-
 test/models/registrar/replace_nameservers_test.rb | 6 +++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/app/models/registrar.rb b/app/models/registrar.rb
index e038cdb16..50ace5d4e 100644
--- a/app/models/registrar.rb
+++ b/app/models/registrar.rb
@@ -148,10 +148,12 @@ class Registrar < ApplicationRecord
       failed_list = []
 
       nameservers.where(hostname: hostname).find_each do |origin|
-        next unless domains.include?(origin.domain.name) || domains.empty?
+        idn = origin.domain.name
+        puny = origin.domain.name_puny
+        next unless domains.include?(idn) || domains.include?(puny) || domains.empty?
 
         if origin.domain.nameservers.where(hostname: new_attributes[:hostname]).any?
-          failed_list << origin.domain.name
+          failed_list << idn
           next
         end
 
@@ -160,7 +162,7 @@ class Registrar < ApplicationRecord
         new_nameserver.attributes = new_attributes
         new_nameserver.save!
 
-        domain_list << origin.domain.name
+        domain_list << idn
 
         origin.destroy!
       end
diff --git a/test/integration/api/nameservers/put_test.rb b/test/integration/api/nameservers/put_test.rb
index 3ab4f4dd4..77b01a9b1 100644
--- a/test/integration/api/nameservers/put_test.rb
+++ b/test/integration/api/nameservers/put_test.rb
@@ -67,7 +67,8 @@ class APINameserversPutTest < ApplicationIntegrationTest
                             attributes: { hostname: 'ns55.bestnames.test',
                                           ipv4: ['192.0.2.55'],
                                           ipv6: ['2001:db8::55'] },
-                    affected_domains: ["airport.test", "shop.test"] }}),
+                    affected_domains: ["airport.test", "shop.test"],
+                    skipped_domains: [] }}),
                  JSON.parse(response.body, symbolize_names: true)
   end
 
diff --git a/test/models/registrar/replace_nameservers_test.rb b/test/models/registrar/replace_nameservers_test.rb
index 36071e3fa..b4c99bba1 100644
--- a/test/models/registrar/replace_nameservers_test.rb
+++ b/test/models/registrar/replace_nameservers_test.rb
@@ -10,7 +10,7 @@ class ReplaceNameserversTest < ActiveSupport::TestCase
                        ipv6: '2001:db8::2' }
     result = @registrar.replace_nameservers('ns1.bestnames.test', new_attributes)
 
-    assert_equal(["airport.test", "shop.test"], result)
+    assert_equal([["airport.test", "shop.test"], []], result)
   end
 
   def test_replace_nameservers_in_bulk_returns_empty_array_for_non_existent_base_nameserver
@@ -18,7 +18,7 @@ class ReplaceNameserversTest < ActiveSupport::TestCase
                        ipv6: '2001:db8::2' }
     result = @registrar.replace_nameservers('ns3.bestnames.test', new_attributes)
 
-    assert_equal([], result)
+    assert_equal([[], []], result)
   end
 
   def test_replace_nameserver_in_bulk_respects_domain_limit_scope
@@ -29,7 +29,7 @@ class ReplaceNameserversTest < ActiveSupport::TestCase
       ipv6: '2001:db8::2' }
 
     result = @registrar.replace_nameservers('ns1.bestnames.test', new_attributes, domains: ['shop.test'])
-    assert_equal(["shop.test"], result)
+    assert_equal([["shop.test"], []], result)
 
     unscoped_domain.reload
     eligible_domain.reload

From af0a0611ce799149ff82cf17a8c6cb774b65da65 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= <karlerik@kreative.ee>
Date: Wed, 30 Dec 2020 12:46:51 +0200
Subject: [PATCH 7/7] Bulk NS change: track untouched domains

---
 .../registrar/nameservers_controller.rb       | 23 +++++++++++--------
 .../v1/registrar/nameservers_controller.rb    |  6 ++---
 app/models/registrar.rb                       |  4 +++-
 config/locales/registrar/nameservers.en.yml   |  2 +-
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/app/controllers/registrar/nameservers_controller.rb b/app/controllers/registrar/nameservers_controller.rb
index 1ab0e335f..52c43bb1d 100644
--- a/app/controllers/registrar/nameservers_controller.rb
+++ b/app/controllers/registrar/nameservers_controller.rb
@@ -48,22 +48,25 @@ class Registrar
       parsed_response = JSON.parse(response.body, symbolize_names: true)
 
       if response.code == '200'
-        notices = [t('.replaced')]
-        notices << "#{t('.affected_domains')}: " \
-                   "#{parsed_response[:data][:affected_domains].join(', ')}"
-        if parsed_response[:data][:skipped_domains]
-          notices << "#{t('.skipped_domains')}: " \
-          "#{parsed_response[:data][:skipped_domains].join(', ')}"
-        end
-
-        flash[:notice] = notices.join(', ')
-        redirect_to registrar_domains_url
+        redirect_to(registrar_domains_url,
+                    flash: { notice: compose_notice_message(parsed_response) })
       else
         @api_errors = parsed_response[:message]
         render file: 'registrar/bulk_change/new', locals: { active_tab: :nameserver }
       end
     end
 
+    def compose_notice_message(res)
+      notices = ["#{t('.replaced')}. #{t('.affected_domains')}: " \
+      "#{res[:data][:affected_domains].join(', ')}"]
+
+      if res[:data][:skipped_domains]
+        notices << "#{t('.skipped_domains')}: #{res[:data][:skipped_domains].join(', ')}"
+      end
+
+      notices.join(', ')
+    end
+
     def domain_list_from_csv
       return [] if params[:puny_file].blank?
 
diff --git a/app/controllers/repp/v1/registrar/nameservers_controller.rb b/app/controllers/repp/v1/registrar/nameservers_controller.rb
index a71f11759..39f076e9b 100644
--- a/app/controllers/repp/v1/registrar/nameservers_controller.rb
+++ b/app/controllers/repp/v1/registrar/nameservers_controller.rb
@@ -6,9 +6,9 @@ module Repp
 
         def update
           affected, errored = current_user.registrar
-                                 .replace_nameservers(hostname,
-                                                      hostname_params[:data][:attributes],
-                                                      domains: domains_from_params)
+                                          .replace_nameservers(hostname,
+                                                               hostname_params[:data][:attributes],
+                                                               domains: domains_from_params)
 
           render_success(data: data_format_for_success(affected, errored))
         rescue ActiveRecord::RecordInvalid => e
diff --git a/app/models/registrar.rb b/app/models/registrar.rb
index 50ace5d4e..168dfdca7 100644
--- a/app/models/registrar.rb
+++ b/app/models/registrar.rb
@@ -144,6 +144,7 @@ class Registrar < ApplicationRecord
   # Audit log is needed, therefore no raw SQL
   def replace_nameservers(hostname, new_attributes, domains: [])
     transaction do
+      domain_scope = domains.dup
       domain_list = []
       failed_list = []
 
@@ -162,13 +163,14 @@ class Registrar < ApplicationRecord
         new_nameserver.attributes = new_attributes
         new_nameserver.save!
 
+        domain_scope.delete_if { |i| i == idn || i == puny }
         domain_list << idn
 
         origin.destroy!
       end
 
       self.domains.where(name: domain_list).find_each(&:update_whois_record) if domain_list.any?
-      [domain_list.uniq.sort, failed_list.uniq.sort]
+      [domain_list.uniq.sort, (domain_scope + failed_list).uniq.sort]
     end
   end
 
diff --git a/config/locales/registrar/nameservers.en.yml b/config/locales/registrar/nameservers.en.yml
index 2e645734d..9c1c2a70e 100644
--- a/config/locales/registrar/nameservers.en.yml
+++ b/config/locales/registrar/nameservers.en.yml
@@ -4,4 +4,4 @@ en:
       update:
         replaced: Nameserver have been successfully replaced
         affected_domains: Affected domains
-        skipped_domains: Skipped domains
+        skipped_domains: Untouched domains