Merge pull request #3626 from codecombat/improve-join-classroom

Make joining classrooms more stable

app/views/courses/CoursesView.coffee

@@ -122,25 +122,9 @@ module.exports = class CoursesView extends RootView
     classroomCourseInstances = new CocoCollection([], { url: "/db/course_instance", model: CourseInstance })
     classroomCourseInstances.fetch({ data: {classroomID: newClassroom.id} })
     @listenToOnce classroomCourseInstances, 'sync', ->
-      # join any course instances in the classroom which are free to join
+      # TODO: Smoother system for joining a classroom and course instances, without requiring page reload,
-      jqxhrs = []
+      # and showing which class was just joined. 
-      for courseInstance in classroomCourseInstances.models
+      document.location.search = '' # Using document.location.reload() causes an infinite loop of reloading
-        course = @courses.get(courseInstance.get('courseID'))
-        if course.get('free')
-          jqxhrs.push = courseInstance.addMember(me.id)
-          courseInstance.sessions = new Backbone.Collection()
-          @courseInstances.add(courseInstance)
-      $.when(jqxhrs...).done =>
-        # This is a hack to work around previous hacks
-        # TODO: Do joinWithCode properly (before page load)
-        # TODO: Do data flow properly (so going to the class URL works and we don't need to just refresh)
-        location.search = ""
-        # @state = null
-        # @render()
-        # location.hash = ''
-        # f = -> location.hash = '#just-added-text'
-        # # quick and dirty scroll to just-added classroom
-        # setTimeout(f, 10)
     
   onClickChangeLanguageLink: ->
     application.tracker?.trackEvent 'Student clicked change language', category: 'Courses'

server/handlers/classroom_handler.coffee

@@ -29,7 +29,6 @@ ClassroomHandler = class ClassroomHandler extends Handler
   getByRelationship: (req, res, args...) ->
     method = req.method.toLowerCase()
     return @inviteStudents(req, res, args[0]) if args[1] is 'invite-members'
-    return @joinClassroomAPI(req, res, args[0]) if method is 'post' and args[1] is 'members'
     return @removeMember(req, res, args[0]) if req.method is 'DELETE' and args[1] is 'members'
     return @getMembersAPI(req, res, args[0]) if args[1] is 'members'
     super(arguments...)
@@ -44,32 +43,6 @@ ClassroomHandler = class ClassroomHandler extends Handler
         cleandocs = (UserHandler.formatEntity(req, doc) for doc in users)
         @sendSuccess(res, cleandocs)
 
-  joinClassroomAPI: (req, res, classroomID) ->
-    return @sendUnauthorizedError(res, 'Cannot join a classroom while anonymous') if req.user.isAnonymous()
-    return @sendBadInputError(res, 'Need an object with a code') unless req.body?.code
-    return @sendForbiddenError(res, 'Cannot join a classroom as a teacher') if req.user.isTeacher()
-    code = req.body.code.toLowerCase()
-    Classroom.findOne {code: code}, (err, classroom) =>
-      return @sendDatabaseError(res, err) if err
-      return @sendNotFoundError(res) if not classroom
-      members = _.clone(classroom.get('members'))
-      if _.any(members, (memberID) -> memberID.equals(req.user.get('_id')))
-        return @sendSuccess(res, @formatEntity(req, classroom))
-      update = { $push: { members : req.user.get('_id')}}
-      classroom.update update, (err) =>
-        return @sendDatabaseError(res, err) if err
-        members.push req.user.get('_id')
-        classroom.set('members', members)
-        # TODO: remove teacher check here after forbidden above
-        if req.user.get('role') not in ['student', 'teacher']
-          User.findById(req.user.get('_id')).exec (err, user) =>
-            user.set('role', 'student')
-            user.save (err, user) =>
-              return @sendDatabaseError(res, err) if err
-              return @sendSuccess(res, @formatEntity(req, classroom))
-        else
-          return @sendSuccess(res, @formatEntity(req, classroom))
-
   removeMember: (req, res, classroomID) ->
     userID = req.body.userID
     return @sendBadInputError(res, 'Input must be a MongoDB ID') unless utils.isID(userID)

server/middleware/classrooms.coffee

@@ -12,6 +12,7 @@ Level = require '../models/Level'
 parse = require '../commons/parse'
 LevelSession = require '../models/LevelSession'
 User = require '../models/User'
+CourseInstance = require '../models/CourseInstance'
 
 module.exports =
   getByOwner: wrap (req, res, next) ->
@@ -50,7 +51,7 @@ module.exports =
       levelMap[level.original] = level
     levels = (levelMap[levelOriginal.toString()] for levelOriginal in levelOriginals)
 
-    res.status(200).send(levels)
+    res.status(200).send(_.filter(levels)) # for dev server where not all levels will be found
 
   fetchLevelsForCourse: wrap (req, res) ->
     classroom = yield database.getDocFromHandle(req, Classroom)
@@ -141,3 +142,35 @@ module.exports =
     database.validateDoc(classroom)
     classroom = yield classroom.save()
     res.status(201).send(classroom.toObject({req: req}))
+
+  join: wrap (req, res) ->
+    unless req.body?.code
+      throw new errors.UnprocessableEntity('Need a code')
+    if req.user.isTeacher()
+      throw new errors.Forbidden('Cannot join a classroom as a teacher')
+    code = req.body.code.toLowerCase()
+    classroom = yield Classroom.findOne({code: code})
+    if not classroom
+      throw new errors.NotFound(res) 
+    members = _.clone(classroom.get('members'))
+    if _.any(members, (memberID) -> memberID.equals(req.user._id))
+      return res.send(classroom.toObject({req: req}))
+    update = { $push: { members : req.user._id }}
+    yield classroom.update(update)
+    members.push req.user._id
+    classroom.set('members', members)
+    
+    # make user role student
+    if not req.user.get('role')
+      req.user.set('role', 'student')
+      yield req.user.save()
+
+    # join any course instances for free courses in the classroom
+    courseIDs = (course._id for course in classroom.get('courses'))
+    courses = yield Course.find({_id: {$in: courseIDs}, free: true})
+    freeCourseIDs = (course._id for course in courses)
+    freeCourseInstances = yield CourseInstance.find({ classroomID: classroom._id, courseID: {$in: freeCourseIDs} }).select('_id')
+    freeCourseInstanceIDs = (courseInstance._id for courseInstance in freeCourseInstances)
+    yield CourseInstance.update({_id: {$in: freeCourseInstanceIDs}}, { $addToSet: { members: req.user._id }})
+    yield User.update({ _id: req.user._id }, { $addToSet: { courseInstances: { $each: freeCourseInstanceIDs } } })
+    res.send(classroom.toObject({req: req}))

server/routes/index.coffee

@@ -60,6 +60,7 @@ module.exports.setup = (app) ->
   app.get('/db/classroom/:handle/courses/:courseID/levels', mw.classrooms.fetchLevelsForCourse)
   app.get('/db/classroom/:handle/member-sessions', mw.classrooms.fetchMemberSessions)
   app.get('/db/classroom/:handle/members', mw.classrooms.fetchMembers) # TODO: Use mw.auth?
+  app.post('/db/classroom/:anything/members', mw.auth.checkLoggedIn(), mw.classrooms.join)
   app.get('/db/classroom/:handle', mw.auth.checkLoggedIn()) # TODO: Finish migrating route, adding now so 401 is returned
 
   CodeLog = require ('../models/CodeLog')

spec/server/functional/classrooms.spec.coffee

@@ -9,6 +9,7 @@ requestAsync = Promise.promisify(request, {multiArgs: true})
 User = require '../../../server/models/User'
 Classroom = require '../../../server/models/Classroom'
 Course = require '../../../server/models/Course'
+CourseInstance = require '../../../server/models/CourseInstance'
 Campaign = require '../../../server/models/Campaign'
 LevelSession = require '../../../server/models/LevelSession'
 Level = require '../../../server/models/Level'
@@ -260,63 +261,50 @@ describe 'PUT /db/classroom', ->
                 expect(res.statusCode).toBe(403)
                 done()
             
-describe 'POST /db/classroom/~/members', ->
+describe 'POST /db/classroom/-/members', ->
-
+  
-  it 'clears database users and classrooms', (done) ->
+  beforeEach utils.wrap (done) ->
-    clearModels [User, Classroom], (err) ->
+    yield utils.clearModels([User, Classroom, Course, Campaign])
-      throw err if err
+    @campaign = new Campaign({levels: {}})
-      done()
+    yield @campaign.save()
-
+    @course = new Course({free: true, campaignID: @campaign._id})
-  it 'adds the signed in user to the list of members in the classroom', (done) ->
+    yield @course.save()
-    loginNewUser (user1) ->
+    @teacher = yield utils.initUser({role: 'teacher'})
-      user1.set('role', 'teacher')
+    yield utils.loginUser(@teacher)
-      user1.save (err) ->
+    [res, body] = yield request.postAsync({uri: classroomsURL, json: { name: 'Classroom 5' } })
-        data = { name: 'Classroom 5' }
-        request.post {uri: classroomsURL, json: data }, (err, res, body) ->
-          classroomCode = body.code
-          classroomID = body._id
-          expect(res.statusCode).toBe(201)
-          loginNewUser (user2) ->
-            url = getURL("/db/classroom/~/members")
-            data = { code: classroomCode }
-            request.post { uri: url, json: data }, (err, res, body) ->
-              expect(res.statusCode).toBe(200)
-              Classroom.findById classroomID, (err, classroom) ->
-                expect(classroom.get('members').length).toBe(1)
-                expect(classroom.get('members')?[0]?.equals(user2.get('_id'))).toBe(true)
-                User.findById user2.get('_id'), (err, user2) ->
-                  expect(user2.get('role')).toBe('student')
-                  done()
-
-  it 'does not work if the user is a teacher', (done) ->
-    loginNewUser (user1) ->
-      user1.set('role', 'teacher')
-      user1.save (err) ->
-        data = { name: 'Classroom 5' }
-        request.post {uri: classroomsURL, json: data }, (err, res, body) ->
-          classroomCode = body.code
-          classroomID = body._id
-          expect(res.statusCode).toBe(201)
-          loginNewUser (user2) ->
-            user2.set('role', 'teacher')
-            user2.save (err, user2) ->
-              url = getURL("/db/classroom/~/members")
-              data = { code: classroomCode }
-              request.post { uri: url, json: data }, (err, res, body) ->
-                expect(res.statusCode).toBe(403)
-                Classroom.findById classroomID, (err, classroom) ->
-                  expect(classroom.get('members').length).toBe(0)
-                  done()
-                  
-  it 'does not work if the user is anonymous', utils.wrap (done) ->
-    yield utils.clearModels([User, Classroom])
-    teacher = yield utils.initUser({role: 'teacher'})
-    yield utils.loginUser(teacher)
-    [res, body] = yield request.postAsync {uri: classroomsURL, json: { name: 'Classroom' } }
     expect(res.statusCode).toBe(201)
-    classroomCode = body.code
+    @classroom = yield Classroom.findById(body._id)
+    [res, body] = yield request.postAsync({uri: getURL('/db/course_instance'), json: { courseID: @course.id, classroomID: @classroom.id }})
+    expect(res.statusCode).toBe(200)
+    @courseInstance = yield CourseInstance.findById(res.body._id)
+    @student = yield utils.initUser()
+    done()
+    
+  it 'adds the signed in user to the classroom and any free courses and sets role to student', utils.wrap (done) ->
+    yield utils.loginUser(@student)
+    url = getURL("/db/classroom/anything-here/members")
+    [res, body] = yield request.postAsync { uri: url, json: { code: @classroom.get('code') } }
+    expect(res.statusCode).toBe(200)
+    classroom = yield Classroom.findById(@classroom.id)
+    expect(classroom.get('members').length).toBe(1)
+    expect(classroom.get('members')?[0]?.equals(@student._id)).toBe(true)
+    student = yield User.findById(@student.id)
+    if student.get('role') isnt 'student'
+      fail('student role should be "student"')
+    unless student.get('courseInstances')?[0].equals(@courseInstance._id)
+      fail('student should be added to the free course instance.')
+    done()
+    
+  it 'returns 403 if the user is a teacher', utils.wrap (done) ->
+    yield utils.loginUser(@teacher)
+    url = getURL("/db/classroom/~/members")
+    [res, body] = yield request.postAsync { uri: url, json: { code: @classroom.get('code') } }
+    expect(res.statusCode).toBe(403)
+    done()
+                  
+  it 'returns 401 if the user is anonymous', utils.wrap (done) ->
     yield utils.becomeAnonymous()
-    [res, body] = yield request.postAsync { uri: getURL("/db/classroom/~/members"), json: { code: classroomCode } }
+    [res, body] = yield request.postAsync { uri: getURL("/db/classroom/-/members"), json: { code: @classroom.get('code') } }
     expect(res.statusCode).toBe(401)
     done()