Add a method get_all_staff_in_all_packages() for more sane querysets

loads/models.py

@@ -790,6 +790,29 @@ def get_all_packages(self, include_hidden=False):
 
         return packages
 
+    def get_all_staff_in_all_packages(self, include_hidden=False):
+        """
+        Get all the staff in all the packages that are relevant for a staff member
+
+        This is usually used to restrict a Staff queryset to sensible lists
+        """
+
+        #TODO: probably want to enable this after more testing if this function but it will just add orphans?
+        #if self.user.is_superuser:
+        #    return Staff.objects.all()
+
+        # Get all the packages the staff member has access first
+        all_packages = self.get_all_packages(include_hidden=include_hidden)
+
+        # Get all the groups pointed to by all of those packages
+        target_groups = Group.objects.filter(workpackage__in=all_packages).distinct()
+
+        # These are user objects
+        users_by_groups = User.objects.all().filter(groups__in=target_groups).distinct().order_by('last_name')
+        # Get the matching staff objects
+        staff = Staff.objects.all().filter(user__in=users_by_groups).distinct().order_by('user__last_name')
+        return staff
+
     def get_examined_programmes(self):
         examined_programmes = Programme.objects.all().filter(examiners=self).distinct()
         return examined_programmes

loads/tests_models.py

@@ -756,3 +756,101 @@ def test_both_regex_user_creates_staff(self):
 
         # Check the invalid user is disabled
         self.assertFalse(invalid_user.is_active)
+
+
+class StaffTestCase(TestCase):
+    """Tests for specific model creation functionality"""
+
+    @override_settings(WAM_STAFF_REGEX=None)
+    @override_settings(WAM_EXTERNAL_REGEX=None)
+    def setUp(self):
+        # Logging is very noisy typically
+        logging.disable(logging.CRITICAL)
+
+        # Create a workpackage
+        packageA = WorkPackage.objects.create(name="A", startdate="2017-09-01", enddate="2018-08-31")
+        packageB = WorkPackage.objects.create(name="B", startdate="2017-09-01", enddate="2018-08-31")
+
+        # Create three groups
+        groupA = Group.objects.create(name="A")
+        groupB1 = Group.objects.create(name="B1")
+        groupB2 = Group.objects.create(name="B2")
+        groupC = Group.objects.create(name="C")
+
+        # Package A has one group, B has 2, the last group is "orphaned"
+        packageA.groups.add(groupA)
+        packageB.groups.add(groupB1)
+        packageB.groups.add(groupB2)
+
+        packageA.save()
+        packageB.save()
+
+        # Create some Users
+        userA_1 = User.objects.create(username="A_1", password="test")
+        userA_2 = User.objects.create(username="A_2", password="test")
+        userB1_1 = User.objects.create(username="B1_1", password="test")
+        userB2_1 = User.objects.create(username="B2_1", password="test")
+        userC = User.objects.create(username="C", password="test")
+
+        userA_1.groups.add(groupA)
+        userA_2.groups.add(groupA)
+        userB1_1.groups.add(groupB1)
+        userB2_1.groups.add(groupB2)
+        userC.groups.add(groupC)
+
+    def tearDown(self):
+        # Put the logging back in place
+        logging.disable(logging.NOTSET)
+
+    def test_staff_all_in_package(self):
+        userA_1 = User.objects.get(username="A_1")
+        userA_2 = User.objects.get(username="A_2")
+        userB1_1 = User.objects.get(username="B1_1")
+        userB2_1 = User.objects.get(username="B2_1")
+        userC = User.objects.get(username="C")
+
+        staffA_1 = Staff.objects.get(user=userA_1)
+        staffA_2 = Staff.objects.get(user=userA_2)
+        staffB1_1 = Staff.objects.get(user=userB1_1)
+        staffB2_1 = Staff.objects.get(user=userB2_1)
+        staffC = Staff.objects.get(user=userC)
+
+        # A_1 should see one package, and two people in it
+        self.assertEqual(len(staffA_1.get_all_packages()), 1)
+        self.assertEqual(len(staffA_1.get_all_staff_in_all_packages()), 2)
+
+        # B_1 should see one package, and two people in it (in two separate groups)
+        self.assertEqual(len(staffB1_1.get_all_packages()), 1)
+        self.assertEqual(len(staffB1_1.get_all_staff_in_all_packages()), 2)
+
+        # C should see nothing
+        self.assertEqual(len(staffC.get_all_packages()), 0)
+        self.assertEqual(len(staffC.get_all_staff_in_all_packages()), 0)
+
+        # Now add A_1 into group B1
+        groupB1 = Group.objects.get(name="B1")
+        userA_1.groups.add(groupB1)
+        # Now A_1 should see two package, and four people in them
+        self.assertEqual(len(staffA_1.get_all_packages()), 2)
+        self.assertEqual(len(staffA_1.get_all_staff_in_all_packages()), 4)
+
+        # Now hide package B
+        packageB = WorkPackage.objects.get(name="B")
+        packageB.hidden = True
+        packageB.save()
+
+        # Now A_1 should see one package, and two people in them if we don't include hidden
+        self.assertEqual(len(staffA_1.get_all_packages(include_hidden=False)), 1)
+        self.assertEqual(len(staffA_1.get_all_staff_in_all_packages(include_hidden=False)), 2)
+        # Now A_1 should see two package, and four people in them if we do
+        self.assertEqual(len(staffA_1.get_all_packages(include_hidden=True)), 2)
+        self.assertEqual(len(staffA_1.get_all_staff_in_all_packages(include_hidden=True)), 4)
+
+        user = User.objects.create_superuser(username="superuser", password="")
+        # Try to get the matching staff object
+        staff = Staff.objects.get(user=user)
+
+        # A Superuser should see all of these people (and doesn't see the orphan, or themselves)
+        self.assertEqual(len(staff.get_all_packages()), 2)
+        self.assertEqual(len(staff.get_all_staff_in_all_packages()), 4)
+

loads/views.py

@@ -1199,11 +1199,12 @@ def module_staff_allocation(request, module_id, package_id):
         if formset.is_valid():
             formset.save(commit=False)
             for form in formset:
-                # Some fields are missing, so don't do a full save yet
-                allocation = form.save(commit=False)
-                # Fix the fields
-                allocation.module = module
-                allocation.package = package
+                if form not in formset.deleted_forms:
+                    # Some fields are missing, so don't do a full save yet
+                    allocation = form.save(commit=False)
+                    # Fix the fields
+                    allocation.module = module
+                    allocation.package = package
                 logger.info("[%s] allocation for %s processed", request.user, allocation.staff)
             # Now do a real save
             formset.save(commit=True)
@@ -1725,11 +1726,12 @@ def staff_module_allocation(request, staff_id, package_id):
         if formset.is_valid():
             formset.save(commit=False)
             for form in formset:
-                # Some fields are missing, so don't do a full save yet
-                allocation = form.save(commit=False)
-                # Fix the fields
-                allocation.staff = staff
-                allocation.package = package
+                if form not in formset.deleted_forms:
+                    # Some fields are missing, so don't do a full save yet
+                    allocation = form.save(commit=False)
+                    # Fix the fields
+                    allocation.staff = staff
+                    allocation.package = package
             # Now do a real save
             formset.save(commit=True)
             logger.info("[%s] edited the module allocation for %s on package %s" %
@@ -1824,16 +1826,17 @@ def projects_details(request, project_id):
     """Allows a Project and allocated staff to be edited"""
 
     # Fetch the staff user associated with the person requesting
-    user_staff = get_object_or_404(Staff, user=request.user)
+    staff = get_object_or_404(Staff, user=request.user)
 
     # And the project we are going to act on
     project = get_object_or_404(Project, pk=project_id)
-    package = user_staff.package
+    package = staff.package
 
     # We usually want to restrict the staff to select to the package, but best honour the possibility
     # that there are staff not in, or no longer in the package, so add them too.
     # Some Database layers can't combine the querysets, because of the existing ordering, so we need the ids
-    package_staff_qs = package.get_all_staff()
+    # TODO: maybe add this to the model layer, something like Staff::AugmentQuerySet(A, B)
+    package_staff_qs = staff.get_all_staff_in_all_packages(include_hidden="True")
     package_staff_pks = package_staff_qs.values_list('pk', flat=True)
     logger.debug("[%s] creating query set: package staff pks: %s" % (request.user, package_staff_pks))
     project_staff_pks = ProjectStaff.objects.filter(project=project).values_list('staff_id', flat=True)
@@ -1863,35 +1866,20 @@ def projects_details(request, project_id):
         if project_form.is_valid():
             project_form.save()
 
-        # Restrain querysets as we did for GET for validation checks.
-        for form in formset:
-            # Get a sane list of staff to pick from.
+        if formset.is_valid():
             formset.form.base_fields['staff'].queryset = combined_staff_qs
-            for form in formset:
-                form.fields['staff'].queryset = combined_staff_qs
             formset.empty_form.fields['staff'].queryset = combined_staff_qs
 
-
-        if formset.is_valid():
             formset.save(commit=False)
-
-            logger.debug("[%s] (admin) formset processing" % (request.user,))
             for form in formset:
-                # Some fields are missing, so don't do a full save yet
-                allocation = form.save(commit=False)
-                # Fix the fields
-                allocation.project = project
-                logger.debug("[%s] (admin) processing project allocation %s, staff %s" % (request.user,
-                                                                             project,
-                                                                             form.cleaned_data.get("staff")))
-
-            for allocation in formset.deleted_objects:
-                logger.debug("[%s] (admin) deleting project allocation %s, staff %s" % (request.user,
-                                                                             allocation.project,
-                                                                             allocation.staff))
-                allocation.delete()
-
+                # Get a sane list of staff to pick from.
+                form.fields['staff'].queryset = combined_staff_qs
 
+                # Some fields are missing, so don't do a full save yet, don't do this on delete items
+                if form not in formset.deleted_forms:
+                    allocation = form.save(commit=False)
+                    # Fix the fields
+                    allocation.project = project
             # Now do a real save
             formset.save(commit=True)
             logger.info("[%s] (admin) edited the details for project %s" % (request.user, project),