From 61bf0e50abb2cf09a5fbd7c6993a79f55ff892e3 Mon Sep 17 00:00:00 2001 From: "jack.coggin" Date: Fri, 1 Dec 2023 17:57:41 +0000 Subject: [PATCH] refactor specs and add module_releases shared context --- app/assets/stylesheets/application.scss | 2 +- app/models/user.rb | 4 +- .../{visit_changes.rb => content_changes.rb} | 15 +--- app/views/learning/_card.html.slim | 2 +- app/views/learning/show.html.slim | 4 +- .../data_analysis/user_overview_spec.rb | 6 +- spec/models/training/module_spec.rb | 8 ++ spec/services/content_changes_spec.rb | 81 +++++++++++++++++++ spec/services/visit_changes_spec.rb | 53 ------------ spec/support/shared/with_module_releases.rb | 12 +++ 10 files changed, 112 insertions(+), 75 deletions(-) rename app/services/{visit_changes.rb => content_changes.rb} (62%) create mode 100644 spec/services/content_changes_spec.rb delete mode 100644 spec/services/visit_changes_spec.rb create mode 100644 spec/support/shared/with_module_releases.rb diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 737bafa5c..f8146af84 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -195,6 +195,6 @@ ul>li>ul>li { white-space: pre-wrap; } -#available .govuk-tag { +#available h2 .govuk-tag { margin-left: govuk-spacing(1); } diff --git a/app/models/user.rb b/app/models/user.rb index 2edfd887d..87a32babe 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -388,8 +388,8 @@ def continue_training_recipient? end # @return [VisitChanges] changes since last visit - def visit_changes - @visit_changes ||= VisitChanges.new(user: self) + def content_changes + @content_changes ||= ContentChanges.new(user: self) end private diff --git a/app/services/visit_changes.rb b/app/services/content_changes.rb similarity index 62% rename from app/services/visit_changes.rb rename to app/services/content_changes.rb index 265f4ccc4..63d37671f 100644 --- a/app/services/visit_changes.rb +++ b/app/services/content_changes.rb @@ -1,27 +1,20 @@ -# Changes since the user's last visit +# Content changes since the user's last visit, powered by Ahoy::Visit # -# ContentChanges (powered by visits) might be more appropriate? -class VisitChanges +class ContentChanges extend Dry::Initializer option :user, required: true # @return [Boolean] def new_modules? - # for consistency change me too + return false if previous_visit.nil? + previous_visit && new_modules.any? end # @param mod [Training::Module] # @return [Boolean] def new_module?(mod) - # chained conditions - previous_visit && - previous_visit.started_at.to_i < mod.first_published_at.to_i && - !user.course.started?(mod) - - # or - # primary condition with two guards return false if previous_visit.nil? || user.course.started?(mod) previous_visit.started_at < mod.first_published_at diff --git a/app/views/learning/_card.html.slim b/app/views/learning/_card.html.slim index ff1d5199b..117cba90d 100644 --- a/app/views/learning/_card.html.slim +++ b/app/views/learning/_card.html.slim @@ -2,7 +2,7 @@ .card-container = training_module_image(mod) - - if current_user.visit_changes.new_module?(mod) + - if current_user.content_changes.new_module?(mod) span.govuk-tag.govuk-tag--ey = t('my_learning.new_tag.card') diff --git a/app/views/learning/show.html.slim b/app/views/learning/show.html.slim index e3971d9a1..124117efa 100644 --- a/app/views/learning/show.html.slim +++ b/app/views/learning/show.html.slim @@ -29,8 +29,8 @@ .govuk-grid-column-full h2.govuk-heading-m | Available modules - - if current_user.visit_changes.new_modules? - span.govuk-tag.section-tag + - if current_user.content_changes.new_modules? + span.govuk-tag = t('my_learning.new_tag.section') - if current_user.course.available_modules.any? .grid-container diff --git a/spec/models/data_analysis/user_overview_spec.rb b/spec/models/data_analysis/user_overview_spec.rb index 36a7db111..496d87128 100644 --- a/spec/models/data_analysis/user_overview_spec.rb +++ b/spec/models/data_analysis/user_overview_spec.rb @@ -2,6 +2,7 @@ RSpec.describe DataAnalysis::UserOverview do include_context 'with progress' + include_context 'with module releases' let(:headers) do [ 'Registration Complete', @@ -73,11 +74,6 @@ let(:release_1) { create(:release) } before do - # create records for the previously released modules completed by the `new_module_mail_recipients` - create(:module_release, release_id: release_1.id, module_position: 1, name: 'alpha') - create(:module_release, release_id: release_1.id, module_position: 2, name: 'bravo') - create(:module_release, release_id: release_1.id, module_position: 3, name: 'charlie') - # create notes for the `with_notes` and `without_notes` users create(:note, user: user_1) create(:note, user: user_2) diff --git a/spec/models/training/module_spec.rb b/spec/models/training/module_spec.rb index d41a4454a..41764342a 100644 --- a/spec/models/training/module_spec.rb +++ b/spec/models/training/module_spec.rb @@ -67,4 +67,12 @@ expect(mod.topic_count).to eq 8 # 4, 1, 3 end end + + describe '#first_published_at' do + include_context 'with module releases' + + it 'returns the first published date' do + expect(mod.first_published_at).to be_within(1.second).of 2.days.ago + end + end end diff --git a/spec/services/content_changes_spec.rb b/spec/services/content_changes_spec.rb new file mode 100644 index 000000000..3b85f40ba --- /dev/null +++ b/spec/services/content_changes_spec.rb @@ -0,0 +1,81 @@ +require 'rails_helper' + +RSpec.describe ContentChanges do + subject(:changes) { described_class.new(user: user) } + + let(:user) { create(:user) } + + include_context 'with module releases' + + describe '#new_modules?' do + context 'without previous visits' do + it 'returns false' do + expect(changes.new_modules?).to be false + end + end + + context 'with visits predating a modules release' do + before do + create :visit, + id: 1, + visitor_token: '123', + user_id: user.id, + started_at: 1.day.ago + + create :visit, + id: 2, + visitor_token: '456', + user_id: user.id, + started_at: 1.minute.ago + end + + it 'returns true' do + expect(changes.new_modules?).to be true + end + end + end + + describe '#new_module?' do + let(:alpha) { Training::Module.by_name(:alpha) } + + context 'without previous visits' do + it 'returns false' do + expect(changes.new_module?(alpha)).to be false + end + end + + context 'with visits since the modules release' do + before do + create :visit, + id: 1, + visitor_token: '123', + user_id: user.id, + started_at: 1.minute.ago + end + + it 'returns false' do + expect(changes.new_module?(alpha)).to be false + end + end + + context 'with visits predating a modules release' do + before do + create :visit, + id: 1, + visitor_token: '123', + user_id: user.id, + started_at: 5.days.ago + + create :visit, + id: 2, + visitor_token: '456', + user_id: user.id, + started_at: 5.days.ago + end + + it 'returns true' do + expect(changes.new_module?(alpha)).to be true + end + end + end +end diff --git a/spec/services/visit_changes_spec.rb b/spec/services/visit_changes_spec.rb deleted file mode 100644 index 8523b9e60..000000000 --- a/spec/services/visit_changes_spec.rb +++ /dev/null @@ -1,53 +0,0 @@ -require 'rails_helper' - -RSpec.describe VisitChanges do - subject(:changes) { described_class.new(user: user) } - - let(:user) { create(:user) } - - # This is now the third spec where we create module releases, does that deserve a shared context? - before do - create(:release, id: 1) - create(:module_release, release_id: 1, module_position: 1, name: 'alpha', first_published_at: 2.days.ago) - - create(:release, id: 2) - create(:module_release, release_id: 2, module_position: 2, name: 'bravo', first_published_at: 3.days.ago) - - create(:release, id: 3) - create(:module_release, release_id: 3, module_position: 3, name: 'charlie', first_published_at: 2.minutes.ago) - end - - # describe '#new_modules?' do - # end - - # describe '#new_module?' do - # end - - describe '#new_modules' do - context 'without previous visits' do - it 'returns no modules' do - expect(changes.new_modules).to be_empty - end - end - - context 'with previous visits' do - before do - create :visit, - id: 1, - visitor_token: '123', - user_id: user.id, - started_at: 1.day.ago - - create :visit, - id: 2, - visitor_token: '456', - user_id: user.id, - started_at: 1.minute.ago - end - - it 'returns newly released modules' do - expect(changes.new_modules.map(&:name)).to eq %w[charlie] - end - end - end -end diff --git a/spec/support/shared/with_module_releases.rb b/spec/support/shared/with_module_releases.rb new file mode 100644 index 000000000..d112e6e0f --- /dev/null +++ b/spec/support/shared/with_module_releases.rb @@ -0,0 +1,12 @@ +RSpec.shared_context 'with module releases' do + before do + create_module_release(1, 'alpha', 2.days.ago) + create_module_release(2, 'bravo', 3.days.ago) + create_module_release(3, 'charlie', 2.minutes.ago) + end + + def create_module_release(id, name, first_published_at) + create(:release, id: id) + create(:module_release, release_id: id, module_position: id, name: name, first_published_at: first_published_at) + end +end