Backup/signups: Reduce str/objectid casting, fix edge case that could break signup

Backend/signups.py

@@ -15,7 +15,6 @@ from itertools import chain
 
 from bson import ObjectId
 from flask import current_app, abort
-from eve.methods.get import getitem_internal
 from eve.methods.patch import patch_internal
 
 
@@ -39,7 +38,11 @@ def wrap_response(function):
 def new_signups(signups):
     """Update the status for all signups to a course."""
     # Remove duplicates by using a set
-    courses = set(item['course'] for item in signups)
+    def get_id(course):
+        """If item has an _id, return it. Otherwise item will be the _id."""
+        return course['_id'] if isinstance(course, dict) else course
+
+    courses = set(get_id(item['course']) for item in signups)
     # Re-format signups into a dict so we can update them easier later
     signups_by_id = {str(item['_id']): item for item in signups}
 
@@ -63,42 +66,46 @@ def patched_signup(update, original):
     """Update status of all signups of the original and new course."""
     # Only need to do something if course is changed
     if 'course' in update:
-        update_signups(str(update['course']))
-        update_signups(str(original['course']))
+        update_signups(update['course'])
+        update_signups(original['course'])
 
 
 def patched_course(update, original):
     """If the number of spots changed, update signups of course."""
     if 'spots' in update:
-        update_signups(str(original['_id']))
+        update_signups(original['_id'])
 
 
 def block_course_deletion(course):
     """If a course has signups, it can't be deleted."""
     count = current_app.data.driver.db['signups'].count({
-        'course': str(course['_id'])
+        'course': course['_id']
     })
 
     if count > 0:
         abort(409, "Course cannot be deleted as long as it has signups.")
 
 
-def update_signups(course):
-    """Update waiting list for all provided courses.
+def update_signups(course_id):
+    """Update waiting list for a course.
+
+    The course id can be given as string or ObjectId.
+
+    We can assume that the course exists, otherwise Eve stops earlier.
 
-    Return list of ids of all signups with modified status.
+    Return list of ids of all reserved signups.
     """
-    # If the course is embedded, we already have the data we need
-    course_data = getitem_internal('courses', _id=str(course))[0]
-    course_id = course_data['_id']
-    total_spots = course_data.get('spots', 0)
+    course_id = ObjectId(course_id)  # Ensure we are working with ObjectId
+
+    # Determine how many spots the course has
+    total_spots = (current_app.data.driver.db['courses']
+                   .find_one({'_id': course_id}, projection={'spots': 1})
+                   .get('spots', 0))
 
     # Next, count current signups not on waiting list
-    collection = current_app.data.driver.db['signups']
-    taken_spots = collection.count({
-        'course': ObjectId(str(course)),
-        'status': {'$ne': 'waiting'}
-    })
+    signups = current_app.data.driver.db['signups']
+    taken_spots = signups.count({'course': course_id,
+                                 'status': {'$ne': 'waiting'}})
 
     available_spots = total_spots - taken_spots
 
@@ -107,15 +114,16 @@ def update_signups(course):
 
     # Finally, get as many signups on the waiting list as spots available
     # sort by _updated, use nethz as tie breaker
-    signups = collection.find({'course': course_id, 'status': 'waiting'},
-                              projection=['_id', 'status'],
-                              sort=[('_updated', 1), ('nethz', 1)],
-                              limit=available_spots)
+    chosen_signups = signups.find({'course': course_id,
+                                   'status': 'waiting'},
+                                  projection=['_id'],
+                                  sort=[('_updated', 1), ('nethz', 1)],
+                                  limit=available_spots)
 
-    signup_ids = [item['_id'] for item in signups]
+    signup_ids = [item['_id'] for item in chosen_signups]
 
-    collection.update_many({'_id': {'$in': signup_ids}},
-                           {'$set': {'status': 'reserved'}})
+    signups.update_many({'_id': {'$in': signup_ids}},
+                        {'$set': {'status': 'reserved'}})
 
     return [str(item) for item in signup_ids]
 

Backend/tests/test_signups.py

@@ -80,8 +80,6 @@ def test_not_enough_spots(app):
                                          data=first,
                                          assert_status=201)
 
-        print(first_response['_updated'])
-
         assert first_response['status'] == 'reserved'
 
         second = {
@@ -93,8 +91,6 @@ def test_not_enough_spots(app):
                                           data=second,
                                           assert_status=201)
 
-        print(second_response['_updated'])
-
         assert second_response['status'] == 'waiting'
 
 
@@ -196,7 +192,7 @@ def course(app):
     with app.admin():
         # Create a few courses to sign up to
         database = app.data.driver.db['courses']
-        yield str(database.insert({'_etag': 'tag'}))
+        yield database.insert({'_etag': 'tag'})
 
 
 @pytest.fixture
@@ -209,43 +205,47 @@ def mock_update():
 def test_post_signups_triggers_update(app, course, mock_update):
     """Test the the update of spots gets triggered correctly."""
     data = {
-        'course': course,
+        'course': str(course),
         'nethz': 'bli'
     }
     app.client.post('signups', data=data, assert_status=201)
-    mock_update.assert_called_with(course)
+
+    # Note: With post, the course comes from the request, not the db,
+    #       so the update will be called with a string
+    mock_update.assert_called_with(str(course))
 
 
 def test_batch_post_signups_triggers_update(app, course, mock_update):
     """Test the the update of spots gets triggered correctly."""
     # We need a second course to test everything
-    other_course = str(app.data.driver.db['courses'].insert({}))
+    other_course = app.data.driver.db['courses'].insert({})
 
     batch = [{
-        'course': course,
+        'course': str(course),
         'nethz': 'bla'
     }, {
-        'course': course,
+        'course': str(course),
         'nethz': 'bli'
     }, {
-        'course': other_course,
+        'course': str(other_course),
         'nethz': 'bli'
     }]
     app.client.post('/signups', data=batch, assert_status=201)
     # Same course doesn't get updated twice per request
-    mock_update.assert_has_calls([call(course), call(other_course)],
+    print(mock_update.mock_calls)
+    mock_update.assert_has_calls([call(str(course)), call(str(other_course))],
                                  any_order=True)
 
 
 def test_patch_signup_triggers_update(app, course, mock_update):
     """Test the the update of spots gets triggered correctly."""
-    fake = str(app.data.driver.db['signups'].insert({
+    fake = app.data.driver.db['signups'].insert({
         '_etag': 'tag',
         'nethz': 'lala',
         'course': 'oldcourse'
-    }))
-    app.client.patch('/signups/' + fake,
-                     data={'course': course},
+    })
+    app.client.patch('/signups/%s' % fake,
+                     data={'course': str(course)},
                      headers={'If-Match': 'tag'},
                      assert_status=200)
 
@@ -255,11 +255,11 @@ def test_patch_signup_triggers_update(app, course, mock_update):
 
 def test_delete_signup_triggers_update(app, course, mock_update):
     """Test the the update of spots gets triggered correctly."""
-    fake = str(app.data.driver.db['signups'].insert({
+    fake = app.data.driver.db['signups'].insert({
         'course': course,
         '_etag': 'tag'
-    }))
-    app.client.delete('/signups/' + fake,
+    })
+    app.client.delete('/signups/%s' % fake,
                       headers={'If-Match': 'tag'},
                       assert_status=204)
     mock_update.assert_called_with(course)
@@ -267,7 +267,7 @@ def test_delete_signup_triggers_update(app, course, mock_update):
 
 def test_patch_course_without_update(app, course, mock_update):
     """Update of spots gets only triggered if number of spots changes."""
-    app.client.patch('/courses/' + course,
+    app.client.patch('/courses/%s' % course,
                      data={'room': 'Something'},
                      headers={'If-Match': 'tag'},
                      assert_status=200)
@@ -276,7 +276,7 @@ def test_patch_course_without_update(app, course, mock_update):
 
 def test_patch_course_with_update(app, course, mock_update):
     """Update of spots gets only triggered if number of spots changes."""
-    app.client.patch('/courses/' + course,
+    app.client.patch('/courses/%s' % course,
                      data={'spots': '10'},
                      headers={'If-Match': 'tag'},
                      assert_status=200)
@@ -285,14 +285,26 @@ def test_patch_course_with_update(app, course, mock_update):
 
 def test_block_delete_ok(app, course):
     """If a course has no signups, it can be deleted."""
-    app.client.delete('/courses/' + course,
+    app.client.delete('/courses/%s' % course,
                       headers={'If-Match': 'tag'},
                       assert_status=204)
 
 
 def test_block_delete_blocked(app, course):
     """If a course has signups, it cannot be deleted."""
-    str(app.data.driver.db['signups'].insert({'course': course}))
-    app.client.delete('/courses/' + course,
+    app.data.driver.db['signups'].insert({'course': course})
+    app.client.delete('/courses/%s' % course,
                       headers={'If-Match': 'tag'},
                       assert_status=409)
+
+
+def test_embed_course(app, course):
+    """Test that embedding course on POST to signups doesn't break anything."""
+    data = {
+        'course': str(course),
+        'nethz': 'bli'
+    }
+
+    app.client.post('signups?embedded={"course": 1}',
+                    data=data,
+                    assert_status=201)