Merge pull request #322 from fasrc/development

Add a PUP note when a zero charge billing record is removed, update Project Permissions

coldfront/core/allocation/models.py

@@ -273,9 +273,10 @@ def cost(self):
         size_attr_name = self._return_size_attr_name()
         if not size_attr_name:
             return None
-        size = self.allocationattribute_set.get(
-                    allocation_attribute_type__name=size_attr_name).value
-        return 0 if not size else price * float(size)
+        size = self.allocationattribute_set.filter(
+                    allocation_attribute_type__name=size_attr_name)
+        size_value = None if not size else size.first().value
+        return 0 if not size_value else price * float(size_value)
 
     @property
     def expires_in(self):

coldfront/core/portal/views.py

@@ -41,11 +41,13 @@ def home(request):
         allocation_list = Allocation.objects.filter(
             Q(status__name__in=['Active', 'New', 'Renewal Requested', ]) &
             Q(project__status__name__in=['Active', 'New']) &
-            Q(project__projectuser__user=request.user) &
-            Q(project__projectuser__status__name__in=['Active', ]) &
-                (Q(project__projectuser__role__name__in=MANAGERS) |
-                Q(allocationuser__user=request.user) &
-                Q(allocationuser__status__name='Active'))
+            (
+                Q(project__projectuser__user=request.user) &
+                Q(project__projectuser__status__name__in=['Active', ]) &
+                    (Q(project__projectuser__role__name__in=MANAGERS) |
+                    Q(allocationuser__user=request.user) &
+                    Q(allocationuser__status__name='Active'))
+            ) | Q(project__pi=request.user)
         ).distinct().order_by('-created')
 
         managed_allocations = Allocation.objects.filter(

coldfront/core/project/forms.py

@@ -79,9 +79,17 @@ class ProjectRemoveUserForm(forms.Form):
 
 class ProjectUserUpdateForm(forms.Form):
     role = forms.ModelChoiceField(
-        queryset=ProjectUserRoleChoice.objects.all(), empty_label=None)
+        queryset=ProjectUserRoleChoice.objects.exclude(name='PI'), empty_label=None)
     enable_notifications = forms.BooleanField(initial=False, required=False)
 
+    def __init__(self, request_user, project_pk, *args, **kwargs):
+        """If user is not PI or Admin, only show Access Manager, Storage Manager, and User roles"""
+        super().__init__(*args, **kwargs)
+        project_obj = get_object_or_404(Project, pk=project_pk)
+        roles = ProjectUserRoleChoice.objects.exclude(name='PI')
+        if not project_obj.pi == request_user and not request_user.is_superuser:
+            self.fields['role'].queryset = roles.exclude(name='General Manager')
+
 
 class ProjectReviewForm(forms.Form):
     reason = forms.CharField(label='Reason for not updating project information', widget=forms.Textarea(attrs={

coldfront/core/project/management/commands/add_default_project_choices.py

@@ -18,7 +18,7 @@ def handle(self, *args, **options):
         for choice in ['Completed', 'Pending', ]:
             ProjectReviewStatusChoice.objects.get_or_create(name=choice)
 
-        for choice in ['User', 'General Manager', 'Storage Manager', 'Access Manager']:
+        for choice in ['User', 'General Manager', 'Storage Manager', 'Access Manager', 'PI']:
             ProjectUserRoleChoice.objects.get_or_create(name=choice)
 
         for choice in ['Active', 'Pending - Add', 'Pending - Remove', 'Denied', 'Removed', ]:

coldfront/core/project/models.py

@@ -23,6 +23,7 @@ class ProjectPermission(Enum):
     USER = 'user'
     DATA_MANAGER = 'data_manager'
     ACCESS_MANAGER = 'access_manager'
+    GENERAL_MANAGER = 'general_manager'
     PI = 'pi'
 
 class ProjectStatusChoice(TimeStampedModel):
@@ -190,23 +191,26 @@ def user_permissions(self, user):
         if user.is_superuser:
             return list(ProjectPermission)
 
-        user_conditions = (models.Q(status__name__in=('Active', 'New')) & models.Q(user=user))
+        user_conditions = (models.Q(status__name='Active') & models.Q(user=user))
         if not self.projectuser_set.filter(user_conditions).exists() and not self.pi.id == user.id:
             return []
 
-        permissions = [ProjectPermission.USER]
 
-        if (
-            self.projectuser_set.filter(user_conditions & models.Q(role__name__in=DATA_MANAGERS)).exists()
-            or self.pi.id == user.id
-        ):
-            permissions.append(ProjectPermission.DATA_MANAGER)
-
-        if (
-            self.projectuser_set.filter(user_conditions & models.Q(role__name__in=ACCESS_MANAGERS)).exists()
-            or self.pi.id == user.id
-        ):
-            permissions.append(ProjectPermission.ACCESS_MANAGER)
+        permissions = [ProjectPermission.USER]
+        project_permission_dict = {
+            ProjectPermission.GENERAL_MANAGER: ["General Manager"],
+            ProjectPermission.DATA_MANAGER: DATA_MANAGERS,
+            ProjectPermission.ACCESS_MANAGER: ACCESS_MANAGERS,
+        }
+
+        for permission, role_list in project_permission_dict.items():
+            if (
+                self.projectuser_set.filter(
+                    user_conditions & models.Q(role__name__in=role_list)
+                ).exists()
+                or self.pi.id == user.id
+            ):
+                permissions.append(permission)
 
         if self.pi.id == user.id:
             permissions.append(ProjectPermission.PI)

coldfront/core/project/templates/project/project_add_users.html

@@ -14,6 +14,7 @@ <h2>Add users to project: {{project.title}}</h2>
 
 <p>
   Adding a user to your project gives them the ability to access and use your project's allocations.
+  If you cannot find the user you want to add in ColdFront, you may need to request their addition via <a href="https://portal.rc.fas.harvard.edu/request/">Portal</a>.
 </p>
 <div class="row">
   <div class="col">

coldfront/core/project/templates/project/project_detail.html

@@ -143,7 +143,7 @@ <h4>Storage &nbsp;
                 </span>
                 </a>
               {% endif %}
-            </td
+            </td>
             {% endif %}
           </tr>
           {% endfor %}
@@ -307,11 +307,11 @@ <h3 class="d-inline" id="users"><i class="fas fa-users" aria-hidden="true"></i>
     <div class="float-right">
       {% if project.status.name != 'Archived' and is_allowed_to_update_project %}
         <a class="btn btn-primary" href="{{mailto}}" role="button"><i class="far fa-envelope" aria-hidden="true"></i> Email Project Users</a>
-        {% if request.user.is_superuser or request.user == project.pi %}
-          <a class="btn btn-success" href="{% url 'project-add-users-search' project.id %}" role="button"><i class="fas fa-user-plus" aria-hidden="true"></i> Add Users</a>
-          <a class="btn btn-danger" href="{% url 'project-remove-users' project.id %}" role="button"><i class="fas fa-user-times" aria-hidden="true"></i> Remove Users</a>
-	{% endif %}
       {% endif %}
+      {% if is_allowed_to_update_users %}
+        <a class="btn btn-success" href="{% url 'project-add-users-search' project.id %}" role="button"><i class="fas fa-user-plus" aria-hidden="true"></i> Add Users</a>
+        <a class="btn btn-danger" href="{% url 'project-remove-users' project.id %}" role="button"><i class="fas fa-user-times" aria-hidden="true"></i> Remove Users</a>
+	    {% endif %}
     </div>
   </div>
   <div class="card-body">
@@ -328,13 +328,15 @@ <h3 class="d-inline" id="users"><i class="fas fa-users" aria-hidden="true"></i>
               </a>
             </th>
             <!-- This is what I want to add - a usage column; -->
-           <th scope="col" class="nosort">
+            <th scope="col" class="nosort">
              <input type="checkbox" class="check" id="selectAll" style="margin-right: 5px;">Notifications On
                <a class="info-button" title="Enable Notifications" data-toggle="popover" data-trigger="click" data-content="When disabled, user will not receive notifications regarding allocation requests and expirations or cloud usage (if applicable).">
                  <i class="fas fa-info-circle" aria-hidden="true"></i>
                </a>
-             </th>
-            <th scope="col">Actions</th>
+            </th>
+            {% if is_allowed_to_update_permissions %}
+              <th scope="col">Actions</th>
+            {% endif %}
           </tr>
         </thead>
         <tbody>
@@ -347,15 +349,14 @@ <h3 class="d-inline" id="users"><i class="fas fa-users" aria-hidden="true"></i>
               <td>{{ user.user.first_name }} {{ user.user.last_name }}</td>
               <td>{{ user.user.email }}</td>
               <td>{{ user.role.name }}</td>
-              <td>
-                {% if is_allowed_to_update_project %}
+              {% if is_allowed_to_update_permissions %}
+                <td>
                   <input type="checkbox"
                   id="email_notifications_for_user_id_{{user.id}}"
                   name="email_notifications_checkbox"
                   value="{{ user.enable_notifications }}"
                   {% if user.enable_notifications %} checked {% endif %}
-                  {% if user.role.name == "Access Manager" %} disabled {% endif %}
-                  {% if not request.user.is_superuser and user.role.name == "User" and request.user == user.user %} disabled {% endif %}>
+                  {% if user.role.name != "User" %} disabled {% endif %}>
                 {% elif request.user == user.user %}
                   <input type="checkbox"
                   id="email_notifications_for_user_id_{{user.id}}"
@@ -369,10 +370,10 @@ <h3 class="d-inline" id="users"><i class="fas fa-users" aria-hidden="true"></i>
                   value="{{ user.enable_notifications }}"
                   {% if user.enable_notifications %} checked {% endif %}
                   disabled>
-                {% endif %}
-              </td>
+                </td>
+              {% endif %}
               <td>
-                {% if is_allowed_to_update_users %}
+                {% if is_allowed_to_update_permissions and user != project.pi %}
                   <a href="{% url 'project-user-detail' project.pk user.id %}"><i class="fas fa-user-edit" aria-hidden="true"></i><span class="sr-only">Edit</span></a>
                 {% endif %}
               </td>

coldfront/core/project/templates/project/project_user_detail.html

@@ -55,10 +55,10 @@ <h3>Project: {{project.title}}</h3>
               <th scope="row">Role:</th>
               <td>{{project_user_update_form.role}}<br/><br/>
                 <b>Users</b> can view information about their project and the project's related allocations.<br/>
-                <b>Access Managers</b> have the permissions to manage group membership and permissions.<br/>
+                <b>Access Managers</b> have the permissions to manage group membership.<br/>
                 <b>Storage Managers</b> have the permissions to make allocation requests.<br/>
-                <b>General Managers</b> have all the permissions of a PI.<br/>
-                <b>PIs</b> have permissions to manage the project and its related allocations, regardless of their assigned projectuser role.<br/>
+                <b>General Managers</b> have the permissions of a PI, save the ability to assign other General Managers.<br/>
+                <b>PIs</b> have permissions to manage the project, its users, and its related allocations regardless of their assigned projectuser role.<br/>
               </td>
             </tr>
             <tr>

coldfront/core/project/test_views.py

@@ -12,11 +12,7 @@
     ProjectStatusChoiceFactory,
     ProjectAttributeTypeFactory,
 )
-from coldfront.core.project.models import (
-    Project,
-    ProjectStatusChoice,
-    ProjectUserStatusChoice,
-)
+from coldfront.core.project.models import Project, ProjectUserStatusChoice
 
 logging.disable(logging.CRITICAL)
 
@@ -116,23 +112,34 @@ def test_projectdetail_request_allocation_button_visibility(self):
         """Test ProjectDetail request allocation button visibility to different projectuser roles"""
         button_text = 'Request New Storage Allocation'
         utils.page_contains_for_user(self, self.admin_user, self.url, button_text) # admin can see request allocation button
-
         utils.page_contains_for_user(self, self.pi_user, self.url, button_text) # pi can see request allocation button
-
         # data manager can see request allocation button
         utils.page_contains_for_user(self, self.proj_datamanager, self.url, button_text)
         # access manager cannot see request allocation button
         utils.page_does_not_contain_for_user(self, self.proj_accessmanager, self.url, button_text)
-
         utils.page_does_not_contain_for_user(self, self.project_user, self.url, button_text) # non-manager user cannot see request allocation button
 
+    def test_projectdetail_adduser_button_visibility(self):
+        """Test ProjectDetail add user button visibility to different projectuser roles"""
+        button_text = 'Add User'
+        # admin can see add user button
+        utils.page_contains_for_user(self, self.admin_user, self.url, button_text)
+        # pi can see add user button
+        utils.page_contains_for_user(self, self.pi_user, self.url, button_text)
+        # access manager can see add user button
+        utils.page_contains_for_user(self, self.proj_accessmanager, self.url, button_text)
+        # storage manager cannot see add user button
+        utils.page_does_not_contain_for_user(self, self.proj_datamanager, self.url, button_text)
+        # non-manager user cannot see add user button
+        utils.page_does_not_contain_for_user(self, self.project_user, self.url, button_text)
+
     def test_projectdetail_edituser_button_visibility(self):
         """Test ProjectDetail edit button visibility to different projectuser roles"""
         utils.page_contains_for_user(self, self.admin_user, self.url, 'fa-user-edit') # admin can see edit button
         utils.page_contains_for_user(self, self.pi_user, self.url, 'fa-user-edit') # pi can see edit button
         utils.page_does_not_contain_for_user(self, self.project_user, self.url, 'fa-user-edit') # non-manager user cannot see edit button
-        # access manager can see edit button
-        utils.page_contains_for_user(self, self.proj_accessmanager, self.url, 'fa-user-edit')
+        # access manager cannot see edit button
+        utils.page_does_not_contain_for_user(self, self.proj_accessmanager, self.url, 'fa-user-edit')
         # data manager cannot see edit button
         utils.page_does_not_contain_for_user(self, self.proj_datamanager, self.url, 'fa-user-edit')
 
@@ -458,3 +465,49 @@ def setUp(self):
     def test_projectuserdetailview_access(self):
         """test access to project user detail page"""
         self.project_access_tstbase(self.url)
+        utils.test_user_can_access(self, self.pi_user, self.url)# pi can access
+        utils.test_user_can_access(self, self.proj_generalmanager, self.url) # general manager can access
+        utils.test_user_cannot_access(self, self.proj_accessmanager, self.url)# access manager cannot access
+        utils.test_user_cannot_access(self, self.proj_datamanager, self.url)# storage manager cannot access
+
+    def test_projectuserdetailview_role_options(self):
+        """Only Admin and PI should see option to set role to General Manager;
+        option to set role to PI should not be available"""
+        self.client.force_login(self.admin_user)
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 200)
+        self.assertIn('project_user_update_form', response.context)
+        form = response.context['project_user_update_form']
+        self.assertIn('role', form.fields)
+        role_field = form.fields['role']
+        role_names = [role.name for role in role_field.choices.queryset]
+        self.assertNotIn('PI', role_names)
+        self.assertIn('General Manager', role_names)
+        self.assertIn('Access Manager', role_names)
+        self.assertIn('Storage Manager', role_names)
+
+        self.client.force_login(self.pi_user)
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 200)
+        self.assertIn('project_user_update_form', response.context)
+        form = response.context['project_user_update_form']
+        self.assertIn('role', form.fields)
+        role_field = form.fields['role']
+        role_names = [role.name for role in role_field.choices.queryset]
+        self.assertNotIn('PI', role_names)
+        self.assertIn('General Manager', role_names)
+        self.assertIn('Access Manager', role_names)
+        self.assertIn('Storage Manager', role_names)
+
+        self.client.force_login(self.proj_generalmanager)
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 200)
+        self.assertIn('project_user_update_form', response.context)
+        form = response.context['project_user_update_form']
+        self.assertIn('role', form.fields)
+        role_field = form.fields['role']
+        role_names = [role.name for role in role_field.choices.queryset]
+        self.assertNotIn('PI', role_names)
+        self.assertNotIn('General Manager', role_names)
+        self.assertIn('Access Manager', role_names)
+        self.assertIn('Storage Manager', role_names)

coldfront/core/project/views.py

@@ -139,6 +139,9 @@ def get_context_data(self, **kwargs):
         context['is_allowed_to_update_users'] = self.object.has_perm(
             self.request.user, ProjectPermission.ACCESS_MANAGER
         )
+        context['is_allowed_to_update_permissions'] = self.object.has_perm(
+            self.request.user, ProjectPermission.GENERAL_MANAGER
+        )
 
         if self.request.user.is_superuser:
             attributes = list(
@@ -879,8 +882,14 @@ class ProjectUserDetail(LoginRequiredMixin, UserPassesTestMixin, TemplateView):
     def test_func(self):
         """UserPassesTestMixin Tests"""
         project_obj = get_object_or_404(Project, pk=self.kwargs.get('pk'))
-        if project_obj.has_perm(self.request.user, ProjectPermission.ACCESS_MANAGER):
-            return True
+        project_user = project_obj.projectuser_set.get(pk=self.kwargs.get('project_user_pk'))
+        if project_obj.has_perm(self.request.user, ProjectPermission.GENERAL_MANAGER):
+            if project_user.user == project_obj.pi:
+                err = 'You cannot edit the PI of the project.'
+                messages.error(self.request, err)
+                return False
+            else:
+                return True
         err = 'You do not have permission to edit project users.'
         messages.error(self.request, err)
         return False
@@ -893,6 +902,7 @@ def get(self, request, *args, **kwargs):
             project_user_obj = project_obj.projectuser_set.get(pk=project_user_pk)
 
             project_user_update_form = ProjectUserUpdateForm(
+                self.request.user, self.kwargs.get('pk'),
                 initial={
                     'role': project_user_obj.role,
                     'enable_notifications': project_user_obj.enable_notifications,
@@ -928,6 +938,7 @@ def post(self, request, *args, **kwargs):
                 )
 
             project_user_update_form = ProjectUserUpdateForm(
+                self.request.user, self.kwargs.get('pk'),
                 request.POST,
                 initial={
                     'role': project_user_obj.role.name,

coldfront/core/test_helpers/factories.py

@@ -368,6 +368,7 @@ def setup_models(test_case):
     # pi is a project admin but not an AllocationUser.
     test_case.pi_user = UserFactory(username='sdpoisson')
     test_case.proj_allocation_user = UserFactory(username='ljbortkiewicz')
+    test_case.proj_generalmanager = UserFactory(username='tedison')
     test_case.proj_datamanager = UserFactory(username='ajayer')
     test_case.proj_accessmanager = UserFactory(username='mdavis')
     test_case.proj_nonallocation_user = UserFactory(username='wkohn')
@@ -398,7 +399,8 @@ def setup_models(test_case):
         AllocationUserFactory(user=user, allocation=test_case.proj_allocation)
 
     for user, role in {
-        test_case.pi_user:'General Manager',
+        test_case.pi_user:'PI',
+        test_case.proj_generalmanager: 'General Manager',
         test_case.proj_datamanager: 'Storage Manager',
         test_case.proj_accessmanager: 'Access Manager',
         test_case.proj_nonallocation_user: 'User',