Merge pull request #5581 from lyttam/BOAC-6703

BOAC-6703: limits peer advisor search to PA notes and comments

Author: mohamalnadi

boac/api/notes_controller.py

@@ -130,8 +130,10 @@ def add_note_comment():
     parent_note = Note.find_by_id(note_id=parent_note_id)
     if not parent_note or not can_current_user_access_note(parent_note):
         raise ResourceNotFoundError('Note not found')
-    pam_membership = get_department_membership_with_role(current_user, 'peer_advisor_manager')
-    peer_advising_department_id = pam_membership.get('peerAdvisingDepartmentId', None) if pam_membership else None
+    peer_advising_department_id = None
+    if (parent_note.peer_advising_department_id):
+        pam_membership = get_department_membership_with_role(current_user, 'peer_advisor_manager')
+        peer_advising_department_id = pam_membership.get('peerAdvisingDepartmentId', None) if pam_membership else None
     comment = create_note_comment(parent_note, params, peer_advising_department_id)
     return tolerant_jsonify(get_boac_note_as_compatible_json(note=comment, note_read=True))
 

boac/api/search_controller.py

@@ -166,11 +166,27 @@ def search_peer_advising_notes():
         )
 
         # The front-end needs full-blown note objects because they are editable by the current-user.
-        note_ids = [n['id'] for n in search_results.get('notes', [])]
+        note_ids = []
+        for n in search_results.get('notes', []):
+            note_ids.append(n['id'])
+            if n['parentNoteId']:
+                note_ids.append(n['parentNoteId'])
         notes_json = []
-        if note_ids:
-            notes = Note.find_by_ids(note_ids)
-            all_sids = [note.sid for note in notes]
+        if len(note_ids):
+            notes_and_comments = Note.find_peer_notes_by_ids(peer_advising_department_id, note_ids)
+            notes = []
+            comments_by_note_id = {}
+            append_note = notes.append
+
+            def append_comment(comment):
+                comment_json = comment.to_api_json()
+                comments_by_note_id.setdefault(comment.parent_note_id, []).append({
+                    **comment_json,
+                    'author': get_note_author_summary(comment_json),
+                })
+            for item in notes_and_comments:
+                (append_comment if item.parent_note_id else append_note)(item)
+            all_sids = list({note.sid for note in notes})
             students_by_sid = {s['sid']: s for s in get_basic_student_data(sids=all_sids)}
 
             for note in notes:
@@ -179,6 +195,7 @@ def search_peer_advising_notes():
                 notes_json.append({
                     **note_json,
                     'author': get_note_author_summary(note_json),
+                    'comments': comments_by_note_id.get(note.id, []),
                     'student': {
                         'sid': student['sid'],
                         'uid': student['uid'],

boac/models/note.py

@@ -130,6 +130,15 @@ def find_by_ids(cls, note_ids):
         criteria = and_(cls.id.in_(note_ids), cls.deleted_at == None)  # noqa: E711
         return cls.query.filter(criteria).all()
 
+    @classmethod
+    def find_peer_notes_by_ids(cls, peer_advising_department_id, note_ids):
+        criteria = and_(
+            cls.id.in_(note_ids),
+            cls.peer_advising_department_id == peer_advising_department_id,
+            cls.deleted_at == None,  # noqa: E711
+        )
+        return cls.query.filter(criteria).all()
+
     @classmethod
     def get_notes_by_parent_id(cls, parent_note_id):
         criteria = and_(cls.parent_note_id == parent_note_id, cls.deleted_at == None)  # noqa: E711

src/stores/search.ts

@@ -54,6 +54,7 @@ export const useSearchStore: StoreDefinition = defineStore('search', {
       this.includeCourses = currentUser.canAccessCanvasData
       this.includeNotes = currentUser.canAccessAdvisingData
       this.includeStudents = true
+      this.isSearching = false
     },
     resetAutocompleteInput() {this.autocompleteInputResetKey++},
     setAuthor(value: string | null) {this.author = value},

tests/conftest.py

@@ -291,6 +291,25 @@ def mock_advising_note(app, db):
     std_commit(allow_test_environment=True)
 
 
+@pytest.fixture
+def mock_ce3_advising_note(app, db):
+    """Create CE3 advising note with attachment (mock s3)."""
+    note = _create_mock_note(
+        app=app,
+        attachment='fixtures/mock_advising_note_attachment_1.txt',
+        author_dept_codes=['ZCEEE'],
+        author_uid='5405613',
+        db=db,
+        topics=['Other / Reason not listed'],
+    )
+    Note.refresh_search_index()
+    std_commit(allow_test_environment=True)
+    yield note
+    Note.delete(note_id=note.id)
+    Note.refresh_search_index()
+    std_commit(allow_test_environment=True)
+
+
 @pytest.fixture
 def mock_advising_note_with_comments(app, db, fake_auth, mock_advising_note):
     """Create advising note with comments."""
@@ -365,6 +384,7 @@ def mock_note_draft(app, db):
     )
     yield note
     Note.delete(note_id=note.id)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
 
 
@@ -446,19 +466,21 @@ def mock_eop_peer_advising_note(fake_auth, db):
         topics=['topic1', 'topic2', 'topic3'],
     )
     db.session.add(note)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
     logout_user()
 
     yield Note.find_by_id(note.id)
 
     Note.delete(note.id)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
 
 
 @pytest.fixture
 def mock_navcal_peer_advising_note(fake_auth, db):
     """Create a CE3 NAVCAL Peer Advising Note."""
-    peer_advisor_author_uid = '1133400'
+    peer_advisor_author_uid = '188444'
     fake_auth.login(peer_advisor_author_uid)
 
     navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
@@ -475,20 +497,53 @@ def mock_navcal_peer_advising_note(fake_auth, db):
         topics=['topic1', 'topic2', 'topic3'],
     )
     db.session.add(note)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
     logout_user()
 
     yield Note.find_by_id(note.id)
 
     Note.delete(note.id)
+    Note.refresh_search_index()
+    std_commit(allow_test_environment=True)
+
+
+@pytest.fixture
+def mock_navcal_peer_advising_manager_note(fake_auth, db):
+    """CE3 NAVCAL Peer Advising Note created by a Peer Advising Manager."""
+    peer_advising_manager_uid = '2525'
+    fake_auth.login(peer_advising_manager_uid)
+
+    navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
+    # Create the note
+    note = Note.create(
+        author_uid=peer_advising_manager_uid,
+        author_name='Grigsby Columbo',
+        author_role='peer_advisor_manager',
+        author_dept_codes=[],
+        sid='9000000000',
+        body='Whoopsy daisy yoo hoo!',
+        peer_advising_department_id=navcal_department.id,
+        subject='Test Note',
+        topics=['topic1', 'topic2', 'topic3'],
+    )
+    db.session.add(note)
+    Note.refresh_search_index()
+    std_commit(allow_test_environment=True)
+    logout_user()
+
+    yield Note.find_by_id(note.id)
+
+    Note.delete(note.id)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
 
 
 @pytest.fixture
 def mock_navcal_peer_advising_note_with_comments(fake_auth, db, mock_navcal_peer_advising_note):
     """Create a NAVCAL Peer Advising Note with comments."""
-    navcal_peer_advisor_author_uid = '1133400'
-    navcal_peer_advisor_uid = '188444'
+    navcal_peer_advisor_author_uid = '188444'
+    navcal_peer_advisor_uid = '1133400'
     navcal_peer_advising_manager_uid = '2525'
     mech_eng_peer_advising_manager_uid = '1133399'
     non_pam_advisor_uid = '242881'
@@ -559,26 +614,27 @@ def mock_navcal_peer_advising_note_with_comments(fake_auth, db, mock_navcal_peer
     db.session.add(advisor_comment)
     logout_user()
 
-    # Add a comment from non-Peer Advising Manager
+    # Add a comment from an advisor who is not a Peer Advisor Manager
     fake_auth.login(non_pam_advisor_uid)
     advisor_comment = Note.create(
         author_uid=non_pam_advisor_uid,
-        author_name='Joni Mitchell',
-        author_role='peer_advisor_manager',
+        author_name='Mort Korn',
+        author_role='advisor',
         author_dept_codes=[],
         sid=mock_navcal_peer_advising_note.sid,
-        body='A comment from a different department.',
+        body='A comment from a regular non-PAM advisor.',
         parent_note_id=mock_navcal_peer_advising_note.id,
         peer_advising_department_id=None,
         subject='',
     )
     db.session.add(advisor_comment)
     logout_user()
-
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
     yield Note.find_by_id(mock_navcal_peer_advising_note.id)
 
     Note.delete(mock_navcal_peer_advising_note.id)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
 
 
@@ -593,8 +649,11 @@ def mock_private_advising_note(app, db):
         db=db,
         is_private=True,
     )
+    Note.refresh_search_index()
+    std_commit(allow_test_environment=True)
     yield note
     Note.delete(note_id=note.id)
+    Note.refresh_search_index()
     std_commit(allow_test_environment=True)
 
 
@@ -668,6 +727,7 @@ def _create_mock_note(
             )
             author_id = AuthorizedUser.get_id_per_uid(author_uid)
             note_reads = NoteRead.find_or_create(author_id, [note.id])
+            Note.refresh_search_index()
             db.session.add(note)
             db.session.add(note_reads[0])
             std_commit(allow_test_environment=True)

tests/test_api/test_peer_advising_notes_controller.py

@@ -374,7 +374,7 @@ def test_peer_advisor_note_comment(self, client, fake_auth, mock_navcal_peer_adv
 
         # Another Peer Advisor comments on the note
         body = 'A very interesting comment!'
-        fake_auth.login(ce3_navcal_peer_advisor_2_uid)
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
         api_json = self._api_add_note_comment(
             body=body,
             client=client,
@@ -498,8 +498,8 @@ def test_authorized_peer_advisor_note_comment(self, client, fake_auth, mock_navc
         # Comment author edits the comment
         body = 'A very interesting comment!'
         comments = Note.get_notes_by_parent_id(mock_navcal_peer_advising_note_with_comments.id)
-        fake_auth.login(ce3_navcal_peer_advisor_2_uid)
-        comment = next((c for c in comments if c.author_uid == ce3_navcal_peer_advisor_2_uid), None)
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
+        comment = next((c for c in comments if c.author_uid == ce3_navcal_peer_advisor_uid), None)
         api_json = self._api_edit_note_comment(
             comment_id=comment.id,
             body=body,

tests/test_api/test_peer_advising_search_controller.py

@@ -28,6 +28,10 @@
 from boac.models.peer_advising_department import PeerAdvisingDepartment
 
 ce3_navcal_peer_advisor_uid = '1133400'
+ce3_pam_advisor_uid = '2525'
+ce3_non_pam_advisor_uid = '5405613'
+coe_mech_peer_advisor_uid = '1913062'
+eop_peer_advisor_uid = '1563405'
 
 
 class TestPeerAdvisingNoteSearch:
@@ -49,13 +53,108 @@ def test_peer_advising_search_with_missing_input_no_options(self, client, fake_a
         navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
         _api_search(client, ' \t  ', peer_advising_department_id=navcal_department.id, expected_status_code=400)
 
-    def test_peer_advising_search_includes_notes_if_requested(self, client, fake_auth):
+    def test_peer_advising_search_notes_no_notes(self, client, fake_auth):
         """Does not include any notes if the notes do not exist."""
         fake_auth.login(ce3_navcal_peer_advisor_uid)
         navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
+        api_json = _api_search(client, 'Anastasia', peer_advising_department_id=navcal_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+    def test_peer_advising_search_notes_no_match(
+            self,
+            client,
+            fake_auth,
+            mock_navcal_peer_advising_note,  # noqa: ARG002
+            mock_navcal_peer_advising_note_with_comments,  # noqa: ARG002
+        ):
+        """Does not include any notes if none match the search query."""
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
+        navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
         api_json = _api_search(client, 'Brigitte', peer_advising_department_id=navcal_department.id)
         self._assert(api_json, note_count=0, student_count=0)
 
+    def test_peer_advising_search_includes_peer_notes(
+            self,
+            client,
+            fake_auth,
+            mock_navcal_peer_advising_note,  # noqa: ARG002
+            mock_navcal_peer_advising_note_with_comments,  # noqa: ARG002
+        ):
+        """Includes notes created by other peer advisors in the same department."""
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
+        navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
+        api_json = _api_search(client, 'Anastasia', peer_advising_department_id=navcal_department.id)
+        self._assert(api_json, note_count=1, student_count=0)
+
+        api_json = _api_search(client, 'a comment', peer_advising_department_id=navcal_department.id)
+        self._assert(api_json, note_count=1, student_count=0)
+
+    def test_peer_advising_search_includes_pam_notes(self, client, fake_auth, mock_navcal_peer_advising_manager_note):  # noqa: ARG002
+        """Includes notes created by a Peer Advisor Manager in the same department."""
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
+        navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
+        api_json = _api_search(client, 'daisy', peer_advising_department_id=navcal_department.id)
+        self._assert(api_json, note_count=1, student_count=0)
+
+    def test_search_excludes_foreign_dept_peer_notes(
+            self,
+            client,
+            fake_auth,
+            mock_navcal_peer_advising_note,  # noqa: ARG002
+            mock_navcal_peer_advising_note_with_comments,  # noqa: ARG002
+        ):
+        """Does not include notes or comments created by peer advisors outside the department."""
+        fake_auth.login(eop_peer_advisor_uid)
+        eop_department = PeerAdvisingDepartment.get_department_by_name('Educational Opportunity Program')
+        api_json = _api_search(client, 'Anastasia', peer_advising_department_id=eop_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+        api_json = _api_search(client, 'comment', peer_advising_department_id=eop_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+    def test_excludes_foreign_dept_pam_notes(self, client, fake_auth, mock_navcal_peer_advising_manager_note):  # noqa: ARG002
+        """Does not include a note created by Peer Advisor Manager outside the department."""
+        fake_auth.login(eop_peer_advisor_uid)
+        eop_department = PeerAdvisingDepartment.get_department_by_name('Educational Opportunity Program')
+        api_json = _api_search(client, 'daisy', peer_advising_department_id=eop_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+    def test_search_excludes_same_dept_non_pam_note(self, client, fake_auth, mock_ce3_advising_note):  # noqa: ARG002
+        """Does not include a peer advising note created by a non-PAM advisor in the same department."""
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
+        navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
+        api_json = _api_search(client, 'darling', peer_advising_department_id=navcal_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+        fake_auth.login(eop_peer_advisor_uid)
+        eop_department = PeerAdvisingDepartment.get_department_by_name('Educational Opportunity Program')
+        api_json = _api_search(client, 'darling', peer_advising_department_id=eop_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+    def test_search_excludes_pam_comment_foreign_dept_peer_note(
+            self,
+            client,
+            fake_auth,
+            mock_navcal_peer_advising_note_with_comments,  # noqa: ARG002
+        ):
+        """Does not include a peer advising note from a different department and commented on by Peer Advisor Manager in the same department."""
+        fake_auth.login(coe_mech_peer_advisor_uid)
+        mech_eng_department = PeerAdvisingDepartment.get_department_by_name('Mechanical Engineering')
+        api_json = _api_search(client, 'a comment', peer_advising_department_id=mech_eng_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
+    def test_search_excludes_pam_comment_non_peer_note(
+            self,
+            client,
+            fake_auth,
+            mock_advising_note_with_comments,  # noqa: ARG002
+        ):
+        """Does not include a non-peer note commented on by a Peer Advisor Manager in the same department."""
+        fake_auth.login(ce3_navcal_peer_advisor_uid)
+        navcal_department = PeerAdvisingDepartment.get_department_by_name('NAVCAL')
+        api_json = _api_search(client, 'a comment', peer_advising_department_id=navcal_department.id)
+        self._assert(api_json, note_count=0, student_count=0)
+
 
 def _api_search(
         client,