Skip to content

Commit

Permalink
Addressed all review comments up until this point. Changed the layout of
Browse files Browse the repository at this point in the history 
the collection learner view to display different sections based on
recommended explorations to play, completed explorations, and future
explorations. Altered a lot of styling and frontend logic/structure
based on all of these suggestions. Updated the URL interpolation service
to throw errors instead of returning an invalid value when the function
is used wrong. Introduced a 'new user' in the backend test_utils for cleaner
tests which deal with testing state on a fresh user (versus abusing one
of the other profiles supplied by test_utils). Renamed the progress
model for collections to 'CollectionProgressModel'. Updated the demo
collection to have a better title.
  • Loading branch information
@BenHenning
BenHenning committed Nov 11, 2015
1 parent 49bcf8c commit 221d4a2
Show file tree
Hide file tree
Showing 20 changed files with 419 additions and 190 deletions.
34 changes: 17 additions & 17 deletions core/controllers/collection_viewer_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
class CollectionViewerPermissionsTest(test_utils.GenericTestBase):
"""Test permissions for learners to view collections."""

COL_ID = 'cid'
COLLECTION_ID = 'cid'

def setUp(self):
"""Before each individual test, create a dummy collection."""
Expand All @@ -36,21 +36,21 @@ def setUp(self):
self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME)
self.EDITOR_ID = self.get_user_id_from_email(self.EDITOR_EMAIL)

self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME)
self.VIEWER_ID = self.get_user_id_from_email(self.VIEWER_EMAIL)
self.signup(self.NEW_USER_EMAIL, self.NEW_USER_USERNAME)
self.NEW_USER_ID = self.get_user_id_from_email(self.NEW_USER_EMAIL)

self.save_new_valid_collection(self.COL_ID, self.EDITOR_ID)
self.save_new_valid_collection(self.COLLECTION_ID, self.EDITOR_ID)

def test_unpublished_collections_are_invisible_to_logged_out_users(self):
response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID),
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 404)

def test_unpublished_collections_are_invisible_to_unconnected_users(self):
self.login(self.VIEWER_EMAIL)
self.login(self.NEW_USER_EMAIL)
response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID),
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 404)
self.logout()
Expand All @@ -64,15 +64,15 @@ def test_unpublished_collections_are_invisible_to_other_editors(self):

self.login(OTHER_EDITOR_EMAIL)
response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID),
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 404)
self.logout()

def test_unpublished_collections_are_visible_to_their_editors(self):
self.login(self.EDITOR_EMAIL)
response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID))
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
self.assertEqual(response.status_int, 200)
self.logout()

Expand All @@ -81,24 +81,24 @@ def test_unpublished_collections_are_visible_to_admins(self):
self.set_admins([self.ADMIN_EMAIL])
self.login(self.ADMIN_EMAIL)
response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID))
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
self.assertEqual(response.status_int, 200)
self.logout()

def test_published_collections_are_visible_to_guests(self):
rights_manager.publish_collection(self.EDITOR_ID, self.COL_ID)
def test_published_collections_are_visible_to_logged_out_users(self):
rights_manager.publish_collection(self.EDITOR_ID, self.COLLECTION_ID)

response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID),
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 200)

def test_published_collections_are_visible_to_anyone_logged_in(self):
rights_manager.publish_collection(self.EDITOR_ID, self.COL_ID)
def test_published_collections_are_visible_to_logged_in_users(self):
rights_manager.publish_collection(self.EDITOR_ID, self.COLLECTION_ID)

self.login(self.VIEWER_EMAIL)
self.login(self.NEW_USER_EMAIL)
response = self.testapp.get(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COL_ID),
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 200)

Expand Down
4 changes: 2 additions & 2 deletions core/controllers/reader_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,15 @@ def test_unpublished_explorations_are_visible_to_admins(self):
self.assertEqual(response.status_int, 200)
self.logout()

def test_published_explorations_are_visible_to_guests(self):
def test_published_explorations_are_visible_to_logged_out_users(self):
rights_manager.publish_exploration(self.EDITOR_ID, self.EXP_ID)

response = self.testapp.get(
'%s/%s' % (feconf.EXPLORATION_URL_PREFIX, self.EXP_ID),
expect_errors=True)
self.assertEqual(response.status_int, 200)

def test_published_explorations_are_visible_to_anyone_logged_in(self):
def test_published_explorations_are_visible_to_logged_in_users(self):
rights_manager.publish_exploration(self.EDITOR_ID, self.EXP_ID)

self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME)
Expand Down
32 changes: 19 additions & 13 deletions core/domain/collection_services.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,21 @@ def get_collection_titles_and_categories(collection_ids):
def get_completed_exploration_ids(user_id, collection_id):
"""Returns a list of explorations the user has completed within the context
of the provided collection. Returns an empty list if the user has not yet
completed any explorations within the collection.
completed any explorations within the collection. Note that this function
will also return an empty list if either the collection and/or user do not
exist.
A progress model isn't added until the first exploration of a collection is
completed, so, if a model is missing, there isn't enough information to
infer whether that means the collection doesn't exist, the user doesn't
exist, or if they just haven't mdae any progress in that collection yet.
Thus, we just assume the user and collection exist for the sake of this
call, so it returns an empty list, indicating that no progress has yet been
made.
"""
completion_model = user_models.ExplorationInCollectionCompletionModel.get(
progress_model = user_models.CollectionProgressModel.get(
user_id, collection_id)
return completion_model.completed_explorations if completion_model else []
return progress_model.completed_explorations if progress_model else []


def get_next_exploration_ids_to_complete_by_user(user_id, collection_id):
Expand All @@ -291,16 +301,12 @@ def get_next_exploration_ids_to_complete_by_user(user_id, collection_id):

def record_played_exploration_in_collection_context(
user_id, collection_id, exploration_id):
completion_model = (
user_models.ExplorationInCollectionCompletionModel.get_or_create(
user_id, collection_id))

completed_explorations = copy.deepcopy(
completion_model.completed_explorations)
if exploration_id not in completed_explorations:
completed_explorations.append(exploration_id)
completion_model.completed_explorations = completed_explorations
completion_model.put()
progress_model = user_models.CollectionProgressModel.get_or_create(
user_id, collection_id)

if exploration_id not in progress_model.completed_explorations:
progress_model.completed_explorations.append(exploration_id)
progress_model.put()


def _get_collection_summary_dicts_from_models(collection_summary_models):
Expand Down
52 changes: 28 additions & 24 deletions core/domain/collection_services_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,10 @@ def test_get_collection_titles_and_categories(self):
})


class ExplorationInCollectionContextUnitTests(CollectionServicesUnitTests):
"""Tests query and recording methods related to explorations which are
played in the context of a collection.
class CollectionProgressUnitTests(CollectionServicesUnitTests):
"""Tests functions which deal with any progress a user has made within a
collection, including query and recording methods related to explorations
which are played in the context of the collection.
"""

COL_ID_0 = '0_collection_id'
Expand All @@ -128,16 +129,15 @@ class ExplorationInCollectionContextUnitTests(CollectionServicesUnitTests):
EXP_ID_1 = '1_exploration_id'
EXP_ID_2 = '2_exploration_id'

def _get_completion_model(self, user_id, collection_id):
return user_models.ExplorationInCollectionCompletionModel.get(
user_id, collection_id)
def _get_progress_model(self, user_id, collection_id):
return user_models.CollectionProgressModel.get(user_id, collection_id)

def _record_completion(self, user_id, collection_id, exploration_id):
collection_services.record_played_exploration_in_collection_context(
user_id, collection_id, exploration_id)

def setUp(self):
super(ExplorationInCollectionContextUnitTests, self).setUp()
super(CollectionProgressUnitTests, self).setUp()

# Create a new collection and exploration.
self.save_new_valid_collection(
Expand All @@ -153,22 +153,23 @@ def setUp(self):
}], 'Added new exploration')

def test_get_completed_exploration_ids(self):
# There should be no exception if the user or collection do not exist.
collection_services.get_completed_exploration_ids(
'Fake', self.COL_ID_0)
collection_services.get_completed_exploration_ids(
self.OWNER_ID, 'Fake')
# There should be no exception if the user or collection do not exist;
# it should also returns an empty list in both of these situations.
self.assertEqual(collection_services.get_completed_exploration_ids(
'Fake', self.COL_ID_0), [])
self.assertEqual(collection_services.get_completed_exploration_ids(
self.OWNER_ID, 'Fake'), [])

# If no model exists, there should be no completed exploration IDs.
self.assertIsNone(
self._get_completion_model(self.OWNER_ID, self.COL_ID_0))
self._get_progress_model(self.OWNER_ID, self.COL_ID_0))
self.assertEqual(collection_services.get_completed_exploration_ids(
self.OWNER_ID, self.COL_ID_0), [])

# If the first exploration is completed, it should be reported.
self._record_completion(self.OWNER_ID, self.COL_ID_0, self.EXP_ID_0)
self.assertIsNotNone(
self._get_completion_model(self.OWNER_ID, self.COL_ID_0))
self._get_progress_model(self.OWNER_ID, self.COL_ID_0))
self.assertEqual(collection_services.get_completed_exploration_ids(
self.OWNER_ID, self.COL_ID_0), [self.EXP_ID_0])

Expand All @@ -183,7 +184,7 @@ def test_get_completed_exploration_ids(self):

def test_get_next_exploration_ids_to_complete_by_user(self):
# This is an integration test depending on
# get_completed_exploration_ids and logic interal collection_domain
# get_completed_exploration_ids and logic interal to collection_domain
# which is tested in isolation in collection_domain_test.

# Setup the connection graph. The user must complete the first
Expand All @@ -202,16 +203,19 @@ def test_get_next_exploration_ids_to_complete_by_user(self):
'new_value': ['0_exp_skill']
}], 'Updated exp 2 to require exp 0 before being playable')

# There should not be an exception if the user does not exist.
collection_services.get_next_exploration_ids_to_complete_by_user(
'Fake', self.COL_ID_0)
# If the user doesn't exist, assume they haven't made any progress on
# the collection. This means the initial explorations should be
# suggested.
self.assertEqual(
collection_services.get_next_exploration_ids_to_complete_by_user(
'Fake', self.COL_ID_0), [self.EXP_ID_0, self.EXP_ID_1])

# There should be an exception if the collection does not exist.
with self.assertRaises(Exception):
collection_services.get_next_exploration_ids_to_complete_by_user(
self.OWNER_ID, 'Fake')

# If the user hasn't made any progress in the colleciton yet, only the
# If the user hasn't made any progress in the collection yet, only the
# initial explorations should be suggested.
self.assertEqual(
collection_services.get_collection_by_id(
Expand Down Expand Up @@ -244,15 +248,15 @@ def test_record_played_exploration_in_collection_context(self):

# By default, no completion model should exist for a given user and
# collection.
completion_model = self._get_completion_model(
completion_model = self._get_progress_model(
self.OWNER_ID, self.COL_ID_0)
self.assertIsNone(completion_model)

# If the user 'completes an exploration', the model should record it.
collection_services.record_played_exploration_in_collection_context(
self.OWNER_ID, self.COL_ID_0, self.EXP_ID_0)

completion_model = self._get_completion_model(
completion_model = self._get_progress_model(
self.OWNER_ID, self.COL_ID_0)
self.assertIsNotNone(completion_model)
self.assertEqual(completion_model.completed_explorations, [
Expand All @@ -262,7 +266,7 @@ def test_record_played_exploration_in_collection_context(self):
# collection, it should not be duplicated.
collection_services.record_played_exploration_in_collection_context(
self.OWNER_ID, self.COL_ID_0, self.EXP_ID_0)
completion_model = self._get_completion_model(
completion_model = self._get_progress_model(
self.OWNER_ID, self.COL_ID_0)
self.assertEqual(completion_model.completed_explorations, [
self.EXP_ID_0])
Expand All @@ -274,7 +278,7 @@ def test_record_played_exploration_in_collection_context(self):
self.OWNER_ID, self.COL_ID_1, self.EXP_ID_0)
collection_services.record_played_exploration_in_collection_context(
self.OWNER_ID, self.COL_ID_1, self.EXP_ID_1)
completion_model = self._get_completion_model(
completion_model = self._get_progress_model(
self.OWNER_ID, self.COL_ID_0)
self.assertEqual(completion_model.completed_explorations, [
self.EXP_ID_0])
Expand All @@ -285,7 +289,7 @@ def test_record_played_exploration_in_collection_context(self):
self.OWNER_ID, self.COL_ID_0, self.EXP_ID_2)
collection_services.record_played_exploration_in_collection_context(
self.OWNER_ID, self.COL_ID_0, self.EXP_ID_1)
completion_model = self._get_completion_model(
completion_model = self._get_progress_model(
self.OWNER_ID, self.COL_ID_0)
self.assertEqual(completion_model.completed_explorations, [
self.EXP_ID_0, self.EXP_ID_2, self.EXP_ID_1])
Expand Down
30 changes: 19 additions & 11 deletions core/storage/user/gae_models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,18 @@ def get(cls, user_id, exploration_id):
instance_id, strict=False)


class ExplorationInCollectionCompletionModel(base_models.BaseModel):
"""Stores all explorations which have been completed within the context of
a collection.
class CollectionProgressModel(base_models.BaseModel):
"""Stores progress a user has made within a collection, including all
explorations which have been completed within the context of the collection.
Please note instances of this progress model will persist even after a
collection is deleted.
TODO(bhenning): Implement a job which removes stale versions of this model
in the data store. That is, it should go through all completion models and
ensure both the user and collection it is associated with still exist within
the data store, otherwise it should remove the instance of the completion
model.
"""

# The user id.
Expand All @@ -165,8 +174,7 @@ def _generate_id(cls, user_id, collection_id):

@classmethod
def create(cls, user_id, collection_id):
"""Creates a new ExplorationInCollectionCompletionModel entry and
returns it.
"""Creates a new CollectionProgressModel entry and returns it.
Note: the client is responsible for actually saving this entity to the
datastore.
Expand All @@ -177,17 +185,17 @@ def create(cls, user_id, collection_id):

@classmethod
def get(cls, user_id, collection_id):
"""Gets the ExplorationInCollectionCompletionModel for the given ids.
"""
"""Gets the CollectionProgressModel for the given ids."""

instance_id = cls._generate_id(user_id, collection_id)
return super(ExplorationInCollectionCompletionModel, cls).get(
return super(CollectionProgressModel, cls).get(
instance_id, strict=False)

@classmethod
def get_or_create(cls, user_id, collection_id):
"""Gets the ExplorationInCollectionCompletionModel for the given ids,
or creates a new entry with the given ids if no such instance yet
exists within the data store.
"""Gets the CollectionProgressModel for the given ids, or creates a new
entry with the given ids if no such instance yet exists within the data
store.
"""
instance_model = cls.get(user_id, collection_id)
if instance_model:
Expand Down
31 changes: 31 additions & 0 deletions core/templates/dev/head/collection_player/CollectionNodeListDirective.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright 2015 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview Directive for creating a list of collection nodes which link to
* playing the exploration in each node.
*
* @author henning.benmax@google.com (Ben Henning)
*/

oppia.directive('collectionNodeListDirective', [function() {
return {
restrict: 'E',
scope: {
getCollectionId: '&collectionId',
getCollectionNodes: '&collectionNodes',
},
templateUrl: 'inline/collection_node_list_directive'
};
}]);
Loading

0 comments on commit 221d4a2

Please sign in to comment.