From 95267951a180ca431e97e207e0ca9f71976a9dd1 Mon Sep 17 00:00:00 2001 From: loiswells97 Date: Mon, 6 Mar 2023 13:53:24 +0000 Subject: [PATCH 1/8] Create draft PR for #143 From 5becec95f063f30048e73d47214876f6d7f5e330 Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Mon, 6 Mar 2023 15:53:43 +0000 Subject: [PATCH 2/8] modifying identifier validations --- app/models/project.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index ace0132ed..af80bdd90 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -4,7 +4,8 @@ class Project < ApplicationRecord before_validation :check_unique_not_null, on: :create - validates :identifier, presence: true, uniqueness: { scope: :locale } + validates :identifier, presence: true, uniqueness: { scope: :locale }, if: :locale + validates :identifier, presence: true, uniqueness: true, unless: :locale belongs_to :parent, class_name: 'Project', foreign_key: 'remixed_from_id', optional: true, inverse_of: :remixes has_many :components, -> { order(default: :desc, name: :asc) }, dependent: :destroy, inverse_of: :project has_many :remixes, class_name: 'Project', foreign_key: 'remixed_from_id', dependent: :nullify, inverse_of: :parent @@ -15,6 +16,14 @@ class Project < ApplicationRecord def check_unique_not_null self.identifier ||= PhraseIdentifier.generate - self.identifier = PhraseIdentifier.generate until Project.find_by(identifier: self.identifier, locale:).nil? + self.identifier = PhraseIdentifier.generate until has_allowed_identifier + end + + def has_allowed_identifier + if self.locale.nil? + Project.find_by(identifier: self.identifier).nil? + else + Project.find_by(identifier: self.identifier, locale:).nil? + end end end From 6ed7c781be7d943cf1a41a0d20e52374bcaef778 Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Mon, 6 Mar 2023 16:38:03 +0000 Subject: [PATCH 3/8] testing the project with nil locale --- spec/models/project_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 96f11a905..b7579bcc9 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -31,5 +31,10 @@ valid_project = build(:project, identifier: saved_project.identifier, locale: 'ja-JP') expect { valid_project.valid? }.not_to change(valid_project, :identifier) end + + it 'changes indentifier if duplicated with nil locale' do + user_project = build(:project, identifier: saved_project.identifier, locale: nil) + expect { user_project.valid? }.to change(user_project, :identifier) + end end end From b3649e9b4d2f1f0061117d09085373e10aae682d Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Tue, 7 Mar 2023 17:37:19 +0000 Subject: [PATCH 4/8] limit identifier to the same user --- app/models/project.rb | 19 +++++++++----- spec/models/project_spec.rb | 47 +++++++++++++++++++++++++++++++---- spec/project_importer_spec.rb | 4 +-- 3 files changed, 57 insertions(+), 13 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index af80bdd90..614c62830 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -4,8 +4,9 @@ class Project < ApplicationRecord before_validation :check_unique_not_null, on: :create - validates :identifier, presence: true, uniqueness: { scope: :locale }, if: :locale - validates :identifier, presence: true, uniqueness: true, unless: :locale + validates :identifier, presence: true, uniqueness: { scope: :locale } + validate :identifier_cannot_be_taken_by_another_user + validates :locale, presence: true, unless: :user_id belongs_to :parent, class_name: 'Project', foreign_key: 'remixed_from_id', optional: true, inverse_of: :remixes has_many :components, -> { order(default: :desc, name: :asc) }, dependent: :destroy, inverse_of: :project has_many :remixes, class_name: 'Project', foreign_key: 'remixed_from_id', dependent: :nullify, inverse_of: :parent @@ -16,14 +17,20 @@ class Project < ApplicationRecord def check_unique_not_null self.identifier ||= PhraseIdentifier.generate - self.identifier = PhraseIdentifier.generate until has_allowed_identifier + self.identifier = PhraseIdentifier.generate until allowed_identifier? end - def has_allowed_identifier - if self.locale.nil? + def allowed_identifier? + if locale.nil? Project.find_by(identifier: self.identifier).nil? else - Project.find_by(identifier: self.identifier, locale:).nil? + Project.find_by(identifier: self.identifier, locale: self.locale).nil? + end + end + + def identifier_cannot_be_taken_by_another_user + if (!Project.where(identifier: self.identifier).where.not(user_id: self.user_id).empty?) + errors.add(:identifier, "can't be taken by another user") end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b7579bcc9..6d93c9181 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -14,6 +14,48 @@ end end + describe 'validations' do + let(:project) { create(:project) } + let(:identifier) { project.identifier } + + it 'is invalid if no user or locale' do + invalid_project = build(:project, locale: nil, user_id: nil) + expect(invalid_project).to be_invalid + end + + it 'is valid if user but no locale' do + valid_project = build(:project, locale: nil) + expect(valid_project).to be_valid + end + + context 'same identifier and same user' do + let(:user_id) { project.user_id } + + it 'is invalid if identifier in use by same user in the same locale' do + new_project = build(:project, identifier: identifier, user_id: user_id, locale: project.locale) + expect(new_project).to be_invalid + end + + it 'is valid if identifier only in use by the user in the another locale' do + new_project = build(:project, identifier: identifier, user_id: user_id, locale: 'another_locale') + expect(new_project).to be_valid + end + end + + context 'same identifier but different user' do + let(:user_id) { 'another_user' } + it 'is invalid if identifier in use by another user in same locale' do + new_project = build(:project, identifier: identifier, user_id: user_id, locale: project.locale) + expect(new_project).to be_invalid + end + + it 'is invalid if identifier in use in another locale by another user' do + new_project = build(:project, identifier: identifier, user_id: user_id, locale: 'another_locale') + expect(new_project).to be_invalid + end + end + end + describe 'check_unique_not_null' do let(:saved_project) { create(:project) } @@ -31,10 +73,5 @@ valid_project = build(:project, identifier: saved_project.identifier, locale: 'ja-JP') expect { valid_project.valid? }.not_to change(valid_project, :identifier) end - - it 'changes indentifier if duplicated with nil locale' do - user_project = build(:project, identifier: saved_project.identifier, locale: nil) - expect { user_project.valid? }.to change(user_project, :identifier) - end end end diff --git a/spec/project_importer_spec.rb b/spec/project_importer_spec.rb index 85a06554f..0e6b07041 100644 --- a/spec/project_importer_spec.rb +++ b/spec/project_importer_spec.rb @@ -21,10 +21,10 @@ end context 'when the project with correct locale does not already exist in the database' do - let(:project) { Project.find_by(identifier: importer.identifier, locale: importer.locale) } + let(:project) { Project.find_by(identifier: importer.identifier, user_id: nil, locale: importer.locale) } before do - create(:project, identifier: importer.identifier) + create(:project, identifier: importer.identifier, user_id: nil) end it 'saves the project to the database' do From 76977e19a7733b53935da51983af4f486097d666 Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Wed, 8 Mar 2023 09:26:47 +0000 Subject: [PATCH 5/8] fixing linting --- app/models/project.rb | 8 ++++---- spec/models/project_spec.rb | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 614c62830..3a92d5358 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -24,13 +24,13 @@ def allowed_identifier? if locale.nil? Project.find_by(identifier: self.identifier).nil? else - Project.find_by(identifier: self.identifier, locale: self.locale).nil? + Project.find_by(identifier: self.identifier, locale:).nil? end end def identifier_cannot_be_taken_by_another_user - if (!Project.where(identifier: self.identifier).where.not(user_id: self.user_id).empty?) - errors.add(:identifier, "can't be taken by another user") - end + return if Project.where(identifier: self.identifier).where.not(user_id:).empty? + + errors.add(:identifier, "can't be taken by another user") end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6d93c9181..433048a24 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -28,29 +28,30 @@ expect(valid_project).to be_valid end - context 'same identifier and same user' do + context 'with same identifier and same user as existing project' do let(:user_id) { project.user_id } it 'is invalid if identifier in use by same user in the same locale' do - new_project = build(:project, identifier: identifier, user_id: user_id, locale: project.locale) + new_project = build(:project, identifier:, user_id:, locale: project.locale) expect(new_project).to be_invalid end it 'is valid if identifier only in use by the user in the another locale' do - new_project = build(:project, identifier: identifier, user_id: user_id, locale: 'another_locale') + new_project = build(:project, identifier:, user_id:, locale: 'another_locale') expect(new_project).to be_valid end end - - context 'same identifier but different user' do + + context 'with same identifier but different user as existing project' do let(:user_id) { 'another_user' } + it 'is invalid if identifier in use by another user in same locale' do - new_project = build(:project, identifier: identifier, user_id: user_id, locale: project.locale) + new_project = build(:project, identifier:, user_id:, locale: project.locale) expect(new_project).to be_invalid end it 'is invalid if identifier in use in another locale by another user' do - new_project = build(:project, identifier: identifier, user_id: user_id, locale: 'another_locale') + new_project = build(:project, identifier:, user_id:, locale: 'another_locale') expect(new_project).to be_invalid end end From 94b892c7fdd43d0979fb5544cb2c6af3fae13f2c Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Wed, 8 Mar 2023 10:53:29 +0000 Subject: [PATCH 6/8] slight refactor of phrase generation --- app/models/project.rb | 9 --------- lib/concepts/project/operations/create_remix.rb | 2 ++ lib/phrase_identifier.rb | 9 ++++++++- spec/models/project_spec.rb | 10 ---------- 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 3a92d5358..165cf513a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -17,15 +17,6 @@ class Project < ApplicationRecord def check_unique_not_null self.identifier ||= PhraseIdentifier.generate - self.identifier = PhraseIdentifier.generate until allowed_identifier? - end - - def allowed_identifier? - if locale.nil? - Project.find_by(identifier: self.identifier).nil? - else - Project.find_by(identifier: self.identifier, locale:).nil? - end end def identifier_cannot_be_taken_by_another_user diff --git a/lib/concepts/project/operations/create_remix.rb b/lib/concepts/project/operations/create_remix.rb index 38a9cdf2c..84360de32 100644 --- a/lib/concepts/project/operations/create_remix.rb +++ b/lib/concepts/project/operations/create_remix.rb @@ -35,6 +35,8 @@ def create_remix(original_project, params, user_id) remix = original_project.dup.tap do |proj| proj.user_id = user_id proj.remixed_from_id = original_project.id + proj.identifier = PhraseIdentifier.generate + proj.locale = nil end original_project.images.each do |image| diff --git a/lib/phrase_identifier.rb b/lib/phrase_identifier.rb index f0aeb96d4..abdccc656 100644 --- a/lib/phrase_identifier.rb +++ b/lib/phrase_identifier.rb @@ -2,6 +2,13 @@ class PhraseIdentifier def self.generate - Word.order(Arel.sql('RANDOM()')).take(3).pluck(:word).join('-') + phrase = Word.order(Arel.sql('RANDOM()')).take(3).pluck(:word).join('-') until unique?(phrase) + phrase + end + + private + + def self.unique?(phrase) + !phrase.nil? && Project.find_by(identifier: phrase).nil? end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 433048a24..0572db25a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -64,15 +64,5 @@ unsaved_project = build(:project, identifier: nil) expect { unsaved_project.valid? }.to change { unsaved_project.identifier.nil? }.from(true).to(false) end - - it 'generates identifier if non-unique within locale' do - invalid_project = build(:project, identifier: saved_project.identifier, locale: saved_project.locale) - expect { invalid_project.valid? }.to change(invalid_project, :identifier) - end - - it 'does not change identifier if duplicated in different locale' do - valid_project = build(:project, identifier: saved_project.identifier, locale: 'ja-JP') - expect { valid_project.valid? }.not_to change(valid_project, :identifier) - end end end From 8614fbab7002aa4e3c7a683291530d2a614dd9b9 Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Wed, 8 Mar 2023 10:57:30 +0000 Subject: [PATCH 7/8] rubocop --- lib/concepts/project/operations/create_remix.rb | 16 ++++++++++------ lib/phrase_identifier.rb | 2 -- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/concepts/project/operations/create_remix.rb b/lib/concepts/project/operations/create_remix.rb index 84360de32..78a103dd0 100644 --- a/lib/concepts/project/operations/create_remix.rb +++ b/lib/concepts/project/operations/create_remix.rb @@ -32,12 +32,7 @@ def remix_project(response, params, user_id, original_project) end def create_remix(original_project, params, user_id) - remix = original_project.dup.tap do |proj| - proj.user_id = user_id - proj.remixed_from_id = original_project.id - proj.identifier = PhraseIdentifier.generate - proj.locale = nil - end + remix = format_project(original_project, user_id) original_project.images.each do |image| remix.images.attach(image.blob) @@ -49,6 +44,15 @@ def create_remix(original_project, params, user_id) remix end + + def format_project(original_project, user_id) + original_project.dup.tap do |proj| + proj.user_id = user_id + proj.remixed_from_id = original_project.id + proj.identifier = PhraseIdentifier.generate + proj.locale = nil + end + end end end end diff --git a/lib/phrase_identifier.rb b/lib/phrase_identifier.rb index abdccc656..5c90f5a86 100644 --- a/lib/phrase_identifier.rb +++ b/lib/phrase_identifier.rb @@ -6,8 +6,6 @@ def self.generate phrase end - private - def self.unique?(phrase) !phrase.nil? && Project.find_by(identifier: phrase).nil? end From 77d3445a81ffa952fb9e5fee98dd5dc68853a556 Mon Sep 17 00:00:00 2001 From: Lois Wells Date: Wed, 8 Mar 2023 10:59:44 +0000 Subject: [PATCH 8/8] extra remix test after refactor --- spec/concepts/project/create_remix_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/concepts/project/create_remix_spec.rb b/spec/concepts/project/create_remix_spec.rb index 3c1307e16..058fe4816 100644 --- a/spec/concepts/project/create_remix_spec.rb +++ b/spec/concepts/project/create_remix_spec.rb @@ -45,6 +45,11 @@ expect(remixed_project.identifier).not_to eq(original_project.identifier) end + it 'sets new project locale to nil' do + remixed_project = create_remix[:project] + expect(remixed_project.locale).to be_nil + end + it 'assigns user_id to new project' do remixed_project = create_remix[:project] expect(remixed_project.user_id).to eq(user_id)