From edd47877c2e58d3b655ff4282ab378e961f57b8d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 19 Oct 2022 20:18:06 +0200
Subject: [PATCH] Extract methods to get tenant subfolder/file paths

We were using the same logic in many different places, so we're
simplifying the code. I'm not convinced about the method names, though,
so we might change them in the future.

Note using this method for the default tenant in the `TenantDiskService`
class resulted in a `//` in the path, which is probably harmless but
very ugly and it also generates a different key than the one we got
until now. I've added an extra test to make sure that isn't the case.
---
 app/models/machine_learning.rb                    | 10 +---------
 app/models/tenant.rb                              | 12 ++++++++++++
 app/views/robots/index.text.erb                   |  6 +-----
 config/sitemap.rb                                 |  7 +------
 lib/active_storage/service/tenant_disk_service.rb |  2 +-
 spec/models/attachable_spec.rb                    |  1 +
 6 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/app/models/machine_learning.rb b/app/models/machine_learning.rb
index 0b680e212..a9504c0a4 100644
--- a/app/models/machine_learning.rb
+++ b/app/models/machine_learning.rb
@@ -89,15 +89,7 @@ class MachineLearning
     end
 
     def tenant_data_folder
-      File.join(tenant_subfolder, "machine_learning", "data").delete_prefix("/")
-    end
-
-    def tenant_subfolder
-      if Tenant.default?
-        ""
-      else
-        File.join("tenants", Tenant.current_schema)
-      end
+      Tenant.path_with_subfolder("machine_learning/data")
     end
 
     def data_output_files
diff --git a/app/models/tenant.rb b/app/models/tenant.rb
index dbe00b335..45d9e2373 100644
--- a/app/models/tenant.rb
+++ b/app/models/tenant.rb
@@ -71,6 +71,18 @@ class Tenant < ApplicationRecord
     end
   end
 
+  def self.subfolder_path
+    if default?
+      ""
+    else
+      File.join("tenants", current_schema)
+    end
+  end
+
+  def self.path_with_subfolder(filename_or_folder)
+    File.join(subfolder_path, filename_or_folder).delete_prefix("/")
+  end
+
   def self.default?
     current_schema == "public"
   end
diff --git a/app/views/robots/index.text.erb b/app/views/robots/index.text.erb
index 8a432d78d..29c1e6ce3 100644
--- a/app/views/robots/index.text.erb
+++ b/app/views/robots/index.text.erb
@@ -14,8 +14,4 @@ Disallow: /*?*locale-switcher
 Disallow: /*?*filter
 Disallow: user_id
 
-<% if Tenant.default? %>
-Sitemap: <%= "#{root_url}sitemap.xml" %>
-<% else %>
-Sitemap: <%= "#{root_url}tenants/#{Tenant.current_schema}/sitemap.xml" %>
-<% end %>
+Sitemap: <%= "#{root_url}#{Tenant.path_with_subfolder("sitemap.xml")}" %>
diff --git a/config/sitemap.rb b/config/sitemap.rb
index 31f973f2b..ecf4378f3 100644
--- a/config/sitemap.rb
+++ b/config/sitemap.rb
@@ -10,12 +10,7 @@ SitemapGenerator::Sitemap.verbose = false if Rails.env.test?
 
 Tenant.run_on_each do
   SitemapGenerator::Sitemap.default_host = root_url(Tenant.current_url_options)
-
-  if Tenant.default?
-    SitemapGenerator::Sitemap.sitemaps_path = nil
-  else
-    SitemapGenerator::Sitemap.sitemaps_path = "tenants/#{Tenant.current_schema}"
-  end
+  SitemapGenerator::Sitemap.sitemaps_path = Tenant.subfolder_path
 
   SitemapGenerator::Sitemap.create do
     add help_path
diff --git a/lib/active_storage/service/tenant_disk_service.rb b/lib/active_storage/service/tenant_disk_service.rb
index 63414f295..836983d03 100644
--- a/lib/active_storage/service/tenant_disk_service.rb
+++ b/lib/active_storage/service/tenant_disk_service.rb
@@ -6,7 +6,7 @@ module ActiveStorage
       if Tenant.default?
         super
       else
-        super.sub(root, File.join(root, "tenants", Tenant.current_schema))
+        super.sub(root, File.join(root, Tenant.subfolder_path))
       end
     end
   end
diff --git a/spec/models/attachable_spec.rb b/spec/models/attachable_spec.rb
index 32b208bc8..2cb13825b 100644
--- a/spec/models/attachable_spec.rb
+++ b/spec/models/attachable_spec.rb
@@ -5,6 +5,7 @@ describe Attachable do
     file_path = build(:image).file_path
 
     expect(file_path).to include "storage/"
+    expect(file_path).not_to include "storage//"
     expect(file_path).not_to include "tenants"
   end