From 947568558b8476e751faa1d6c896d4ca145dc018 Mon Sep 17 00:00:00 2001 From: jack-coggin <119428483+jack-coggin@users.noreply.github.com> Date: Tue, 5 Dec 2023 13:22:51 +0000 Subject: [PATCH] ER-467 New Module Indicator (#924) * check if module has been released since user's last visit * update account page for one login * Add new module indicator tag * revert account page changes * temporarily remove conditional logic from tags to aid reviewing * fix merge conflicts * prevent any available or complete modules from showing new module indicator * add check for module release record to new mod method * rubocop * move styling to stylesheet * revert schema changes * move new module logic to new service * VisitChanges #new_modules without previous visits returns no modules with previous visits returns newly released modules * add started guard clause to new tag * guarding alternatives * refactor specs and add module_releases shared context * add module_release spec * add test for new module indicator on in progress modules * update account page for one login * remove unnecessary visits from spec * revert account changes * Merge remote-tracking branch 'origin/main' into new-module-indicator * revert github workflows * revert erroneous changes * revert changes to course_progress * undo revert to course_progress --------- Co-authored-by: Peter David Hamilton --- app/assets/stylesheets/application.scss | 4 + app/assets/stylesheets/card.scss | 10 +++ app/models/training/module.rb | 5 ++ app/models/user.rb | 5 ++ app/services/content_changes.rb | 34 ++++++++ app/services/course_progress.rb | 22 ++--- app/views/learning/_card.html.slim | 4 + app/views/learning/show.html.slim | 6 +- config/locales/en.yml | 3 + spec/lib/seed_snippets_spec.rb | 2 +- spec/models/module_release_spec.rb | 10 +++ spec/models/training/module_spec.rb | 8 ++ spec/services/content_changes_spec.rb | 92 +++++++++++++++++++++ spec/support/shared/with_module_releases.rb | 12 +++ 14 files changed, 199 insertions(+), 18 deletions(-) create mode 100644 app/services/content_changes.rb create mode 100644 spec/models/module_release_spec.rb create mode 100644 spec/services/content_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 a87d09295..f8146af84 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -194,3 +194,7 @@ ul>li>ul>li { .white-space-pre-wrap { white-space: pre-wrap; } + +#available h2 .govuk-tag { + margin-left: govuk-spacing(1); +} diff --git a/app/assets/stylesheets/card.scss b/app/assets/stylesheets/card.scss index 866e80a83..e7b1cd3cd 100644 --- a/app/assets/stylesheets/card.scss +++ b/app/assets/stylesheets/card.scss @@ -54,6 +54,11 @@ Progress bar max-width: 300px; } +.govuk-tag--ey { + width: min-content; + margin: govuk-spacing(1) 0; +} + &-container { display: flex; flex-direction: column; @@ -108,6 +113,11 @@ Progress bar .bar-ball { background-color: govuk-colour('yellow'); } + + .govuk-tag { + background-color: govuk-colour('light-grey'); + color: govuk-colour('blue'); + } } &:hover &-link--retake { diff --git a/app/models/training/module.rb b/app/models/training/module.rb index fdedbe50b..7a7dd1bac 100644 --- a/app/models/training/module.rb +++ b/app/models/training/module.rb @@ -60,6 +60,11 @@ def debug_summary SUMMARY end + # @return [DateTime, nil] + def first_published_at + ModuleRelease.find_by(module_position: position)&.first_published_at + end + # entry references --------------------------------- # @example diff --git a/app/models/user.rb b/app/models/user.rb index 3875c3e83..f38fc30e3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -387,6 +387,11 @@ def continue_training_recipient? old_visits.pluck(:user_id).include?(id) end + # @return [VisitChanges] changes since last visit + def content_changes + @content_changes ||= ContentChanges.new(user: self) + end + private # @return [Hash] diff --git a/app/services/content_changes.rb b/app/services/content_changes.rb new file mode 100644 index 000000000..cc4279f82 --- /dev/null +++ b/app/services/content_changes.rb @@ -0,0 +1,34 @@ +# Content changes since the user's last visit, powered by Ahoy::Visit +# +class ContentChanges + extend Dry::Initializer + + option :user, required: true + + # @return [Boolean] + def new_modules? + return false if previous_visit.nil? + + new_modules.any? + end + + # @param mod [Training::Module] + # @return [Boolean] + def new_module?(mod) + return false if previous_visit.nil? || user.course.started?(mod) + + previous_visit.started_at < mod.first_published_at + end + + # @return [Array] + def new_modules + user.course.available_modules.select { |mod| new_module?(mod) } + end + +private + + # @return [Ahoy::Visit] + def previous_visit + user.visits.order(started_at: :desc).second + end +end diff --git a/app/services/course_progress.rb b/app/services/course_progress.rb index a51f5d833..46e7ff716 100644 --- a/app/services/course_progress.rb +++ b/app/services/course_progress.rb @@ -29,7 +29,7 @@ def completed_modules # @return [Boolean] def course_completed? - published_modules.all? { |mod| completed?(mod) } + Training::Module.live.all? { |mod| completed?(mod) } end # @return [Array] @@ -58,8 +58,6 @@ def completed?(mod) module_progress(mod).completed? end -private - # @param mod [Training::Module] # @return [Boolean] module content has been viewed def started?(mod) @@ -68,6 +66,8 @@ def started?(mod) module_progress(mod).started? end +private + # @param mod [Training::Module] # @return [Boolean] def active?(mod) @@ -84,24 +84,14 @@ def upcoming?(mod) # @return [Array] training modules by state def by_state(state) case state.to_sym - when :active then training_modules.select { |mod| active?(mod) } - when :upcoming then training_modules.select { |mod| upcoming?(mod) } - when :completed then training_modules.select { |mod| completed?(mod) } + when :active then Training::Module.ordered.select { |mod| active?(mod) } + when :upcoming then Training::Module.ordered.select { |mod| upcoming?(mod) } + when :completed then Training::Module.ordered.select { |mod| completed?(mod) } else raise 'CourseProgress#by_state can query either :active, :upcoming or :completed modules' end end - # @return [Array] training modules with finalised content - def published_modules - training_modules.reject(&:draft?) - end - - # @return [Array] - def training_modules - @training_modules ||= Training::Module.ordered - end - # @return [ModuleProgress] def module_progress(mod) ModuleProgress.new(user: user, mod: mod) diff --git a/app/views/learning/_card.html.slim b/app/views/learning/_card.html.slim index 533865ca6..117cba90d 100644 --- a/app/views/learning/_card.html.slim +++ b/app/views/learning/_card.html.slim @@ -2,6 +2,10 @@ .card-container = training_module_image(mod) + - if current_user.content_changes.new_module?(mod) + span.govuk-tag.govuk-tag--ey + = t('my_learning.new_tag.card') + h3.govuk-heading-s = govuk_link_to mod.card_title, training_module_path(mod.name), no_visited_state: true, class: 'card-link--header' diff --git a/app/views/learning/show.html.slim b/app/views/learning/show.html.slim index c42c2caf2..124117efa 100644 --- a/app/views/learning/show.html.slim +++ b/app/views/learning/show.html.slim @@ -27,7 +27,11 @@ hr.govuk-section-break.govuk-section-break--visible.govuk-section-break--l #available.govuk-grid-row .govuk-grid-column-full - h2.govuk-heading-m Available modules + h2.govuk-heading-m + | Available modules + - 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 - current_user.course.available_modules.each do |mod| diff --git a/config/locales/en.yml b/config/locales/en.yml index 6a261b5d7..24f335c0b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -276,6 +276,9 @@ en: percentage: "Your progress: %{num}" viewed: You have read %{num} pages remaining: Only %{num} pages remaining + new_tag: + card: NEW + section: NEW MODULE AVAILABLE my_learning_log: title: Learning log diff --git a/spec/lib/seed_snippets_spec.rb b/spec/lib/seed_snippets_spec.rb index 75a4ac39a..6a09ff376 100644 --- a/spec/lib/seed_snippets_spec.rb +++ b/spec/lib/seed_snippets_spec.rb @@ -5,7 +5,7 @@ subject(:locales) { described_class.new.call } it 'converts all translations' do - expect(locales.count).to be 187 + expect(locales.count).to be 189 end it 'dot separated key -> Page::Resource#name' do diff --git a/spec/models/module_release_spec.rb b/spec/models/module_release_spec.rb new file mode 100644 index 000000000..f1af6dd49 --- /dev/null +++ b/spec/models/module_release_spec.rb @@ -0,0 +1,10 @@ +require 'rails_helper' + +RSpec.describe ModuleRelease, type: :model do + describe '.ordered' do + include_context 'with module releases' + it 'returns module releases in order of module position' do + expect(described_class.ordered.map(&:module_position)).to eq([1, 2, 3]) + end + end +end 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..042d32fda --- /dev/null +++ b/spec/services/content_changes_spec.rb @@ -0,0 +1,92 @@ +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 + + context 'and a module in progress' do + include_context 'with progress' + before do + start_module(alpha) + end + + it 'returns false' do + expect(changes.new_module?(alpha)).to be false + end + 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