Merge pull request #3158 from exercism/watermark-bug

Fix unread count bug in stream filters

lib/exercism/team_stream_filters.rb

@@ -113,6 +113,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
       SQL
@@ -177,6 +178,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY ex.language
@@ -249,6 +251,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY ex.slug
@@ -324,6 +327,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY u.username
@@ -389,6 +393,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY ex.language

lib/exercism/track_stream_filters.rb

@@ -103,6 +103,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY ex.language
@@ -192,6 +193,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY ex.slug
@@ -273,6 +275,7 @@ def watermarks_sql
             SELECT exercise_id
             FROM views
             WHERE user_id=#{viewer_id}
+            AND last_viewed_at > ex.last_activity_at
           )
           AND mark.at > ex.last_activity_at
         GROUP BY u.username

test/exercism/team_stream_filters_test.rb

@@ -6,7 +6,7 @@ class TeamStreamFiltersTest < Minitest::Test
   def setup
     super
     Language.instance_variable_set(:"@by_track_id", "go" => "Go", "elixir" => "Elixir")
-    Stream.instance_variable_set(:"@ordered_slugs", "go" => %w(leap clock anagram))
+    Stream.instance_variable_set(:"@ordered_slugs", "go" => %w(leap clock anagram gigasecond))
   end
 
   def teardown
@@ -26,10 +26,12 @@ def test_filters
     # Go
     create_exercise(alice, 'go', 'leap', nobody, mon)
     create_exercise(alice, 'go', 'clock', alice.id, mon)
+    create_exercise(alice, 'go', 'gigasecond', alice.id, mon, -10)
 
     create_exercise(bob, 'go', 'leap', nobody, mon)
     create_exercise(bob, 'go', 'clock', bob.id, tue)
     create_exercise(bob, 'go', 'anagram', alice.id, mon)
+    create_exercise(bob, 'go', 'gigasecond', alice.id, mon, -10)
 
     create_exercise(charlie, 'go', 'leap', nobody, mon)
     create_exercise(charlie, 'go', 'clock', nobody, thu)
@@ -41,6 +43,7 @@ def test_filters
     user_ids = [alice.id, bob.id, charlie.id]
 
     Watermark.create!(user_id: alice.id, track_id: 'go', slug: 'clock', at: wed)
+    Watermark.create!(user_id: alice.id, track_id: 'go', slug: 'gigasecond', at: wed)
 
     # Team filters ignore ACLs.
 
@@ -53,7 +56,7 @@ def test_filters
 
     assert_equal 'All', item.text
     assert_equal '/teams/teamster/streams', item.url
-    assert_equal 9, item.total
+    assert_equal 11, item.total
     assert_equal 5, item.unread
 
     # Track filter, no track currently selected.
@@ -70,7 +73,7 @@ def test_filters
 
     assert_equal 'Go', item2.text
     assert_equal '/teams/teamster/streams/tracks/go', item2.url
-    assert_equal 7, item2.total
+    assert_equal 9, item2.total
     assert_equal 4, item2.unread
     refute item2.active?
 
@@ -87,7 +90,7 @@ def test_filters
 
     # Problems in Go, none currently selected.
     filter = TeamStream::ProblemFilter.new(alice.id, user_ids, 'teamster', 'go', nil)
-    assert_equal 3, filter.items.size
+    assert_equal 4, filter.items.size
 
     item1, item2, item3 = filter.items
 
@@ -111,9 +114,9 @@ def test_filters
 
     # Problems in Go, Anagram selected.
     filter = TeamStream::ProblemFilter.new(alice.id, user_ids, 'teamster', 'go', 'anagram')
-    assert_equal 3, filter.items.size
+    assert_equal 4, filter.items.size
 
-    item1, item2, item3 = filter.items
+    item1, item2, item3, item4 = filter.items
 
     assert_equal 'Leap', item1.text
     refute item1.active?
@@ -124,20 +127,23 @@ def test_filters
     assert_equal 'Anagram', item3.text
     assert item3.active?
 
+    assert_equal 'Gigasecond', item4.text
+    refute item4.active?
+
     # Users, none currently selected
     filter = TeamStream::UserFilter.new(alice.id, user_ids, 'teamster', nil)
     assert_equal 3, filter.items.size
 
     item1, item2, item3 = filter.items
     assert_equal 'alice', item1.text
     assert_equal '/teams/teamster/streams/users/alice', item1.url
-    assert_equal 2, item1.total
+    assert_equal 3, item1.total
     assert_equal 1, item1.unread
     refute item1.active?
 
     assert_equal 'bob', item2.text
     assert_equal '/teams/teamster/streams/users/bob', item2.url
-    assert_equal 4, item2.total
+    assert_equal 5, item2.total
     assert_equal 2, item2.unread
     refute item2.active?
 
@@ -175,7 +181,7 @@ def test_filters
 
     assert_equal 'Go', item2.text
     assert_equal '/teams/teamster/streams/users/bob/tracks/go', item2.url
-    assert_equal 3, item2.total
+    assert_equal 4, item2.total
     assert_equal 1, item2.unread
     refute item2.active?
 
@@ -194,11 +200,11 @@ def test_filters
 
   private
 
-  def create_exercise(user, track_id, slug, viewed_by, timestamp)
+  def create_exercise(user, track_id, slug, viewed_by, timestamp, diff = 10)
     exercise = UserExercise.create!(user_id: user.id, language: track_id, slug: slug, iteration_count: 1, last_activity_at: timestamp)
     Submission.create!(user_id: user.id, user_exercise_id: exercise.id, language: track_id, slug: slug)
     unless viewed_by.nil?
-      View.create(user_id: viewed_by, exercise_id: exercise.id, last_viewed_at: timestamp + 10)
+      View.create(user_id: viewed_by, exercise_id: exercise.id, last_viewed_at: timestamp + diff)
     end
     exercise
   end

test/exercism/track_stream_filters_test.rb

@@ -39,10 +39,17 @@ def test_filters
     # access to via the ACL table, because people share links.
     # These views should not be counted.
     #
-    # To do this we want to make sure that we always have:
+    # We also use watermarks to mark a bunch of exercises in the
+    # same track/problem as 'read' with a single database row.
+    #
+    # To do this we want to make sure that for each filter we have:
     # - at least one exercise that is unread
     # - at least one acl exercise that has been viewed
     # - at least one non-acl exercise that has been viewed
+    # - at least one exercise that is unread but under the watermark
+    # - at least one exercise that has a view timestamp that is older
+    #   than the last activity on the exercise, and which has a
+    #   watermark that is newer.
     #
     # We also need to make sure that the counts are correct in
     # the non-active category.
@@ -56,17 +63,19 @@ def test_filters
     # - at least one exercise in an equivalent category that is viewed
 
     # Go
-    create_exercise(alice, 'go', 'leap', alice.id, mon)
-    create_exercise(alice, 'go', 'clock', bob.id, mon)
-    create_exercise(alice, 'go', 'anagram', nobody, mon)
-
-    create_exercise(bob, 'go', 'leap', nobody, mon)
-    create_exercise(bob, 'go', 'clock', alice.id, tue)
-    create_exercise(bob, 'go', 'anagram', bob.id, mon)
-    create_exercise(bob, 'go', 'triangle', nobody, mon)
-
-    create_exercise(charlie, 'go', 'leap', nobody, mon)
-    create_exercise(charlie, 'go', 'clock', bob.id, thu)
+    create_exercise(alice, 'go', 'leap', alice.id, mon) # read (directly)
+    create_exercise(alice, 'go', 'clock', bob.id, mon) # read (watermark)
+    create_exercise(alice, 'go', 'anagram', nobody, mon) # unread
+    create_exercise(alice, 'go', 'gigasecond', alice.id, tue, -10) # read (earlier, then watermark)
+
+    create_exercise(bob, 'go', 'leap', nobody, mon) # unread
+    create_exercise(bob, 'go', 'clock', alice.id, tue) # read (watermark)
+    create_exercise(bob, 'go', 'anagram', bob.id, mon) # unread
+    create_exercise(bob, 'go', 'triangle', nobody, mon) # NO ACL
+    create_exercise(bob, 'go', 'gigasecond', alice.id, mon, -10) # read (earlier, then watermark)
+
+    create_exercise(charlie, 'go', 'leap', nobody, mon) # unread
+    create_exercise(charlie, 'go', 'clock', bob.id, thu) # unroad
     create_exercise(charlie, 'go', 'triangle', alice.id, mon) # viewed (directly) without ACL
 
     # Elixir
@@ -83,13 +92,15 @@ def test_filters
       Problem.new('go', 'leap'),
       Problem.new('go', 'clock'),
       Problem.new('go', 'anagram'),
+      Problem.new('go', 'gigasecond'),
       Problem.new('elixir', 'leap'),
       Problem.new('elixir', 'clock'),
     ].each do |problem|
       ACL.authorize(alice, problem)
     end
 
     Watermark.create!(user_id: alice.id, track_id: 'go', slug: 'clock', at: wed)
+    Watermark.create!(user_id: alice.id, track_id: 'go', slug: 'gigasecond', at: wed)
 
     filter = TrackStream::TrackFilter.new(alice.id, 'go')
     assert_equal 2, filter.items.size
@@ -104,14 +115,14 @@ def test_filters
 
     assert_equal 'Go', item2.text
     assert_equal '/tracks/go/exercises', item2.url
-    assert_equal 8, item2.total
+    assert_equal 10, item2.total
     assert_equal 5, item2.unread
     assert item2.active?
 
     filter = TrackStream::ProblemFilter.new(alice.id, 'go', 'clock')
-    assert_equal 3, filter.items.size
+    assert_equal 4, filter.items.size
 
-    item1, item2, item3 = filter.items
+    item1, item2, item3, item4 = filter.items
 
     assert_equal 'Leap', item1.text
     assert_equal '/tracks/go/exercises/leap', item1.url
@@ -131,6 +142,12 @@ def test_filters
     assert_equal 2, item3.unread
     refute item3.active?, "anagram should not be active"
 
+    assert_equal 'Gigasecond', item4.text
+    assert_equal '/tracks/go/exercises/gigasecond', item4.url
+    assert_equal 2, item4.total
+    assert_equal 0, item4.unread
+    refute item4.active?, "gigasecond should not be active"
+
     only_mine = true
     filter = TrackStream::ViewerFilter.new(alice.id, 'go', only_mine)
     assert_equal 1, filter.items.size
@@ -139,18 +156,18 @@ def test_filters
 
     assert_equal 'My Solutions', item.text
     assert_equal '/tracks/go/my_solutions', item.url
-    assert_equal 3, item.total
+    assert_equal 4, item.total
     assert_equal 1, item.unread
     assert item.active?
   end
 
   private
 
-  def create_exercise(user, track_id, slug, viewed_by, timestamp)
+  def create_exercise(user, track_id, slug, viewed_by, timestamp, diff = 10)
     exercise = UserExercise.create!(user_id: user.id, language: track_id, slug: slug, iteration_count: 1, last_activity_at: timestamp)
     Submission.create!(user_id: user.id, user_exercise_id: exercise.id, language: track_id, slug: slug)
     unless viewed_by.nil?
-      View.create(user_id: viewed_by, exercise_id: exercise.id, last_viewed_at: timestamp + 10)
+      View.create(user_id: viewed_by, exercise_id: exercise.id, last_viewed_at: timestamp + diff)
     end
     exercise
   end