Refactor Chapter Ordering Logic20171015

The bug impacting the CCB integration was due to the "Introduction"
chapter having a higher "score" than every other chapter. It was a
mistake to use Score to compared chapter progress, particularly since
there are more chapters than scores.

This change gathers the chapter ordering logic, which was scattered
throughout the code into a new Chapters enum. Playlist and Chapter now
use Chapters as a key, instead of loose strings. Same for the
ProgressReporter interface.

5 files changed, 64 insertions, 47 deletions

diff --git a/src/test/java/com/p4square/grow/backend/resources/TrainingRecordResourceTest.java b/src/test/java/com/p4square/grow/backend/resources/TrainingRecordResourceTest.java
index db85051..8a052ef 100644
--- a/src/test/java/com/p4square/grow/backend/resources/TrainingRecordResourceTest.java
+++ b/src/test/java/com/p4square/grow/backend/resources/TrainingRecordResourceTest.java
@@ -7,6 +7,7 @@ package com.p4square.grow.backend.resources;
 import java.util.Map;
 import java.util.HashMap;
 
+import com.p4square.grow.model.Chapters;
 import org.restlet.data.Method;
 import org.restlet.Request;
 import org.restlet.Response;
@@ -46,13 +47,13 @@ public class TrainingRecordResourceTest extends ResourceTestBase {
         mResponse = new Response(mRequest);
 
         Playlist playlist = new Playlist();
-        playlist.add("introduction", "intro-1");
-        playlist.add("seeker", "seeker-1");
-        playlist.add("believer", "believer-1");
-        playlist.add("believer", "believer-2");
-        playlist.add("disciple", "disciple-1");
-        playlist.add("teacher", "teacher-1");
-        playlist.add("leader", "leader-1");
+        playlist.add(Chapters.INTRODUCTION, "intro-1");
+        playlist.add(Chapters.SEEKER, "seeker-1");
+        playlist.add(Chapters.BELIEVER, "believer-1");
+        playlist.add(Chapters.BELIEVER, "believer-2");
+        playlist.add(Chapters.DISCIPLE, "disciple-1");
+        playlist.add(Chapters.TEACHER, "teacher-1");
+        playlist.add(Chapters.LEADER, "leader-1");
         mApplication.setDefaultPlaylist(playlist);
     }
 
diff --git a/src/test/java/com/p4square/grow/ccb/CCBProgressReporterTest.java b/src/test/java/com/p4square/grow/ccb/CCBProgressReporterTest.java
index 63a973a..7a2688c 100644
--- a/src/test/java/com/p4square/grow/ccb/CCBProgressReporterTest.java
+++ b/src/test/java/com/p4square/grow/ccb/CCBProgressReporterTest.java
@@ -2,6 +2,7 @@ package com.p4square.grow.ccb;
 
 import com.p4square.ccbapi.CCBAPI;
 import com.p4square.ccbapi.model.*;
+import com.p4square.grow.model.Chapters;
 import org.easymock.Capture;
 import org.easymock.EasyMock;
 import org.junit.Before;
@@ -65,7 +66,7 @@ public class CCBProgressReporterTest {
                 .andReturn(growLevelDate).anyTimes();
         EasyMock.expect(cache.getIndividualPulldownByLabel(GROW_LEVEL))
                 .andReturn(growLevelPulldown).anyTimes();
-        EasyMock.expect(cache.getPulldownItemByName(LookupTableType.UDF_IND_PULLDOWN_1, "Believer"))
+        EasyMock.expect(cache.getPulldownItemByName(LookupTableType.UDF_IND_PULLDOWN_1, "believer"))
                 .andReturn(believer).anyTimes();
 
         // Setup the Grow Assessment field.
@@ -81,7 +82,7 @@ public class CCBProgressReporterTest {
                 .andReturn(growAssessmentDate).anyTimes();
         EasyMock.expect(cache.getIndividualPulldownByLabel(ASSESSMENT_LEVEL))
                 .andReturn(growAssessmentPulldown).anyTimes();
-        EasyMock.expect(cache.getPulldownItemByName(LookupTableType.UDF_IND_PULLDOWN_2, "Believer"))
+        EasyMock.expect(cache.getPulldownItemByName(LookupTableType.UDF_IND_PULLDOWN_2, "believer"))
                 .andReturn(believer).anyTimes();
     }
 
@@ -115,7 +116,7 @@ public class CCBProgressReporterTest {
         replay();
 
         // Test reporter
-        reporter.reportChapterComplete(user, "Believer", date);
+        reporter.reportChapterComplete(user, Chapters.BELIEVER, date);
 
         // Assert that the profile was updated.
         verify();
@@ -138,7 +139,7 @@ public class CCBProgressReporterTest {
         replay();
 
         // Test reporter
-        reporter.reportChapterComplete(user, "Believer", date);
+        reporter.reportChapterComplete(user, Chapters.BELIEVER, date);
 
         // Assert that the profile was updated.
         verify();
@@ -157,7 +158,7 @@ public class CCBProgressReporterTest {
         replay();
 
         // Test reporter
-        reporter.reportChapterComplete(user, "Believer", date);
+        reporter.reportChapterComplete(user, Chapters.BELIEVER, date);
 
         // Assert that the profile was updated.
         verify();
@@ -173,17 +174,21 @@ public class CCBProgressReporterTest {
         replay();
 
         // Test reporter
-        reporter.reportChapterComplete(user, "Believer", date);
+        reporter.reportChapterComplete(user, Chapters.BELIEVER, date);
 
         // Assert that the profile was updated.
         verify();
     }
 
+    /**
+     * Verify that the date is updated even if the level can't be found in the pulldown menu.
+     * @throws Exception
+     */
     @Test
     public void testReportChapterCompleteNoSuchValue() throws Exception {
         // Setup mocks
         setupCacheMocks();
-        EasyMock.expect(cache.getPulldownItemByName(LookupTableType.UDF_IND_PULLDOWN_1, "Foo"))
+        EasyMock.expect(cache.getPulldownItemByName(LookupTableType.UDF_IND_PULLDOWN_1, "seeker"))
                 .andReturn(null).anyTimes();
         Capture<UpdateIndividualProfileRequest> reqCapture = EasyMock.newCapture();
         EasyMock.expect(api.updateIndividualProfile(EasyMock.capture(reqCapture)))
@@ -191,7 +196,7 @@ public class CCBProgressReporterTest {
         replay();
 
         // Test reporter
-        reporter.reportChapterComplete(user, "Foo", date);
+        reporter.reportChapterComplete(user, Chapters.SEEKER, date);
 
         // Assert that the profile was updated.
         verify();
diff --git a/src/test/java/com/p4square/grow/model/PlaylistTest.java b/src/test/java/com/p4square/grow/model/PlaylistTest.java
index 9c893f6..b6650ef 100644
--- a/src/test/java/com/p4square/grow/model/PlaylistTest.java
+++ b/src/test/java/com/p4square/grow/model/PlaylistTest.java
@@ -29,15 +29,15 @@ public class PlaylistTest {
     public void testPlaylistAndChapter() {
         // Create a playlist for the test
         Playlist playlist = new Playlist();
-        playlist.add("chapter1", "video1");
-        playlist.add("chapter1", "video2");
+        playlist.add(Chapters.SEEKER, "video1");
+        playlist.add(Chapters.SEEKER, "video2");
 
         // Chapter should not be complete
-        assertFalse(playlist.isChapterComplete("chapter1"));
+        assertFalse(playlist.isChapterComplete(Chapters.SEEKER));
 
         // We should find the chapter in the map
-        Map<String, Chapter> chapterMap = playlist.getChaptersMap();
-        Chapter chapter1 = chapterMap.get("chapter1");
+        Map<Chapters, Chapter> chapterMap = playlist.getChaptersMap();
+        Chapter chapter1 = chapterMap.get(Chapters.SEEKER);
         assertTrue(null != chapter1);
 
         // We should find the videos in the map.
@@ -53,8 +53,10 @@ public class PlaylistTest {
         video2.complete();
 
         // Chapter should be complete now.
-        assertTrue(playlist.isChapterComplete("chapter1"));
-        assertFalse(playlist.isChapterComplete("bogusChapter"));
+        assertTrue(playlist.isChapterComplete(Chapters.SEEKER));
+        
+        // But other chapters should not be complete.
+        assertFalse(playlist.isChapterComplete(Chapters.BELIEVER));
     }
 
     /**
@@ -75,15 +77,15 @@ public class PlaylistTest {
     @Test
     public void testMergePlaylist() {
         Playlist oldList = new Playlist();
-        oldList.add("chapter1", "video1").setRequired(true);
-        oldList.add("chapter2", "video2").setRequired(false);
-        oldList.add("chapter2", "video3").complete();
+        oldList.add(Chapters.SEEKER, "video1").setRequired(true);
+        oldList.add(Chapters.BELIEVER, "video2").setRequired(false);
+        oldList.add(Chapters.BELIEVER, "video3").complete();
         oldList.setLastUpdated(new Date(100));
 
         Playlist newList = new Playlist();
-        newList.add("chapter1", "video4").setRequired(true);
-        newList.add("chapter2", "video5").setRequired(false);
-        newList.add("chapter3", "video6").setRequired(false);
+        newList.add(Chapters.SEEKER, "video4").setRequired(true);
+        newList.add(Chapters.BELIEVER, "video5").setRequired(false);
+        newList.add(Chapters.DISCIPLE, "video6").setRequired(false);
         newList.setLastUpdated(new Date(500));
 
         // Verify that you can't merge the old into the new
@@ -102,9 +104,9 @@ public class PlaylistTest {
         assertFalse(oldList.find("video6").getRequired());
 
         // New Chapter added
-        Map<String, Chapter> chapters = oldList.getChaptersMap();
+        Map<Chapters, Chapter> chapters = oldList.getChaptersMap();
         assertEquals(3, chapters.size());
-        assertTrue(null != chapters.get("chapter3"));
+        assertTrue(null != chapters.get(Chapters.DISCIPLE));
 
         // Date updated
         assertEquals(newList.getLastUpdated(), oldList.getLastUpdated());
@@ -121,17 +123,17 @@ public class PlaylistTest {
     @Test
     public void testMergeMoveVideoRecord() {
         Playlist oldList = new Playlist();
-        oldList.add("chapter1", "video1").setRequired(true);
-        VideoRecord toMove = oldList.add("chapter1", "video2");
+        oldList.add(Chapters.SEEKER, "video1").setRequired(true);
+        VideoRecord toMove = oldList.add(Chapters.SEEKER, "video2");
         toMove.setRequired(true);
         toMove.complete();
-        oldList.add("chapter2", "video3").complete();
+        oldList.add(Chapters.BELIEVER, "video3").complete();
         oldList.setLastUpdated(new Date(100));
 
         Playlist newList = new Playlist();
-        newList.add("chapter1", "video1").setRequired(true);
-        newList.add("chapter2", "video2").setRequired(true);
-        newList.add("chapter3", "video3").complete();
+        newList.add(Chapters.SEEKER, "video1").setRequired(true);
+        newList.add(Chapters.BELIEVER, "video2").setRequired(true);
+        newList.add(Chapters.DISCIPLE, "video3").complete();
         newList.setLastUpdated(new Date(500));
 
         // Merge the new list into the old and verify results
@@ -143,12 +145,12 @@ public class PlaylistTest {
         assertTrue(oldList.find("video3").getComplete());
 
         // toMove is in the correct chapter.
-        assertNull(oldList.getChaptersMap().get("chapter1").getVideoRecord("video2"));
-        VideoRecord afterMove = oldList.getChaptersMap().get("chapter2").getVideoRecord("video2");
+        assertNull(oldList.getChaptersMap().get(Chapters.SEEKER).getVideoRecord("video2"));
+        VideoRecord afterMove = oldList.getChaptersMap().get(Chapters.BELIEVER).getVideoRecord("video2");
         assertSame(toMove, afterMove);
 
         // video3 got moved to the new chapter3
-        assertNull(oldList.getChaptersMap().get("chapter2").getVideoRecord("video3"));
-        assertTrue(oldList.getChaptersMap().get("chapter3").getVideoRecord("video3").getComplete());
+        assertNull(oldList.getChaptersMap().get(Chapters.BELIEVER).getVideoRecord("video3"));
+        assertTrue(oldList.getChaptersMap().get(Chapters.DISCIPLE).getVideoRecord("video3").getComplete());
     }
 }
diff --git a/src/test/java/com/p4square/grow/model/ScoreTest.java b/src/test/java/com/p4square/grow/model/ScoreTest.java
index 5c7be46..80066fa 100644
--- a/src/test/java/com/p4square/grow/model/ScoreTest.java
+++ b/src/test/java/com/p4square/grow/model/ScoreTest.java
@@ -64,6 +64,15 @@ public class ScoreTest {
     }
 
     /**
+     * Verify that numericScore() throws if a non-score is passed in.
+     */
+    @Test(expected =  IllegalArgumentException.class)
+    public void testInvalidScoreString() {
+        // Introduction is not a valid score.
+        Score.numericScore("introduction");
+    }
+
+    /**
      * Verify that toString() returns the correct mappings.
      */
     @Test
diff --git a/src/test/java/com/p4square/grow/model/TrainingRecordTest.java b/src/test/java/com/p4square/grow/model/TrainingRecordTest.java
index 43caa90..a34befd 100644
--- a/src/test/java/com/p4square/grow/model/TrainingRecordTest.java
+++ b/src/test/java/com/p4square/grow/model/TrainingRecordTest.java
@@ -52,15 +52,15 @@ public class TrainingRecordTest {
         assertEquals(null, r);
 
         // isChapterComplete
-        assertTrue(playlist.isChapterComplete("seeker")); // Complete because not required.
-        assertTrue(playlist.isChapterComplete("disciple")); // Required and completed.
-        assertFalse(playlist.isChapterComplete("teacher")); // Not complete.
+        assertTrue(playlist.isChapterComplete(Chapters.SEEKER)); // Complete because not required.
+        assertTrue(playlist.isChapterComplete(Chapters.DISCIPLE)); // Required and completed.
+        assertFalse(playlist.isChapterComplete(Chapters.TEACHER)); // Not complete.
 
         // getChapterStatuses
-        Map<String, Boolean> statuses = playlist.getChapterStatuses();
-        assertTrue(statuses.get("seeker")); // Complete because not required.
-        assertTrue(statuses.get("disciple")); // Required and completed.
-        assertFalse(statuses.get("teacher")); // Not complete.
+        Map<Chapters, Boolean> statuses = playlist.getChapterStatuses();
+        assertTrue(statuses.get(Chapters.SEEKER)); // Complete because not required.
+        assertTrue(statuses.get(Chapters.DISCIPLE)); // Required and completed.
+        assertFalse(statuses.get(Chapters.TEACHER)); // Not complete.
     }
 
     /**