fix: fap reviewer want to see all proposals in their fap (#739)

apps/backend/src/auth/ProposalAuthorization.ts

@@ -118,6 +118,24 @@ export class ProposalAuthorization {
       });
   }
 
+  async isMemberOfFapProposal(agent: UserWithRole | null, proposalPk: number) {
+    if (agent == null || !agent.id || !agent.currentRole) {
+      return false;
+    }
+
+    const fapsUserIsMemberOf =
+      await this.fapDataSource.getUserFapsByRoleAndFapId(
+        agent.id,
+        agent.currentRole
+      );
+
+    const fapIdsUserIsMemberOf = fapsUserIsMemberOf.map((fap) => fap.id);
+    const faps = await this.fapDataSource.getFapsByProposalPk(proposalPk);
+    const fapIds = faps.map((fap) => fap.id);
+
+    return fapIds.some((id) => fapIdsUserIsMemberOf.includes(id));
+  }
+
   async isScientistToProposal(agent: UserJWT | null, proposalPk: number) {
     if (agent == null || !agent.id) {
       return false;
@@ -216,7 +234,8 @@ export class ProposalAuthorization {
       (await this.isInstrumentManagerToProposal(agent, proposal.primaryKey)) ||
       isInternalReviewerOnSomeTechnicalReview ||
       (await this.isChairOrSecretaryOfProposal(agent, proposal.primaryKey)) ||
-      (await this.isVisitorOfProposal(agent, proposal.primaryKey))
+      (await this.isVisitorOfProposal(agent, proposal.primaryKey)) ||
+      (await this.isMemberOfFapProposal(agent, proposal.primaryKey))
     );
   }
 

apps/backend/src/auth/ReviewAuthorization.ts

@@ -73,6 +73,13 @@ export class ReviewAuthorization {
     if (isReviewerOfProposal) {
       return true;
     }
+    const isMemberOfFap = await this.userAuth.isMemberOfFap(
+      agent,
+      review.fapID
+    );
+    if (isMemberOfFap) {
+      return true;
+    }
 
     return false;
   }

apps/backend/src/datasources/ReviewDataSource.ts

@@ -23,6 +23,13 @@ export interface ReviewDataSource {
     instrumentId?: number,
     submitted?: number
   ): Promise<Review[]>;
+  getAllUsersReviews(
+    fapIds: number[],
+    userId?: number,
+    callId?: number,
+    instrumentId?: number,
+    submitted?: number
+  ): Promise<Review[]>;
   getAssignmentReview(
     fapId: number,
     proposalPk: number,

apps/backend/src/datasources/mockups/FapDataSource.ts

@@ -159,7 +159,7 @@ export class FapDataSourceMock implements FapDataSource {
     fapId?: number
   ): Promise<Fap[]> {
     if (userId && role) {
-      return dummyFaps;
+      return userId === 3 ? [] : dummyFaps;
     }
 
     return [];

apps/backend/src/datasources/mockups/ReviewDataSource.ts

@@ -85,7 +85,12 @@ export class ReviewDataSourceMock implements ReviewDataSource {
   async getProposalReviews(id: number): Promise<Review[]> {
     return [dummyReview];
   }
-  async getUserReviews(fapIds: number[]): Promise<Review[]> {
+  async getUserReviews(fapIds: number[], userId?: number): Promise<Review[]> {
+    if (userId === 3) return [];
+
+    return [dummyReview];
+  }
+  async getAllUsersReviews(fapIds: number[]): Promise<Review[]> {
     return [dummyReview];
   }
 }

apps/backend/src/datasources/postgres/ReviewDataSource.ts

@@ -336,4 +336,64 @@ export default class PostgresReviewDataSource implements ReviewDataSource {
         return reviews.map((review) => createReviewObject(review));
       });
   }
+
+  /*Brief explanation of the query used in getAllUsersReviews
+    This query gets executed when a reviewer wishes to see all the proposals
+    (proposals where they are assigned as a reviewer and where they are not) 
+    in a FAP. Logged in reviewer's id will be returned as the reviewer against 
+    a proposal if the proposal is being reviewed by him and others. Reviewers
+    other than him will be ignored for that proposal.
+  */
+  async getAllUsersReviews(
+    fapIds: number[],
+    userId?: number,
+    callId?: number,
+    instrumentId?: number,
+    status?: ReviewStatus
+  ): Promise<Review[]> {
+    return database
+      .select('fapReviewsTemp.*')
+      .from(
+        database
+          .select('fap_reviews.*')
+          .from('fap_reviews')
+          .modify((qb) => {
+            if (callId) {
+              qb.join('proposals', {
+                'proposals.proposal_pk': 'fap_reviews.proposal_pk',
+              });
+              qb.where('proposals.call_id', callId);
+            }
+            if (instrumentId) {
+              qb.join('instrument_has_proposals', {
+                'instrument_has_proposals.proposal_pk':
+                  'fap_reviews.proposal_pk',
+              });
+              qb.where('instrument_has_proposals.instrument_id', instrumentId);
+            }
+
+            if (status !== null) {
+              qb.where('fap_reviews.status', status);
+            }
+          })
+          .whereIn('fap_id', fapIds)
+          .as('fapReviewsTemp')
+      )
+      .modify((query) =>
+        query
+          .where('user_id', userId)
+          .orWhereNotIn(
+            'proposal_pk',
+            database
+              .select('proposal_pk')
+              .from('fap_reviews')
+              .where('user_id', userId)
+              .distinctOn('proposal_pk')
+          )
+          .distinctOn('proposal_pk')
+      )
+      .then((reviews: ReviewRecord[]) => {
+        return reviews.map((review) => createReviewObject(review));
+      });
+  }
 }

apps/backend/src/queries/ReviewQueries.spec.ts

@@ -3,8 +3,8 @@ import { container } from 'tsyringe';
 
 import { dummyReview } from '../datasources/mockups/ReviewDataSource';
 import {
+  dummyUserNotOnProposalWithRole,
   dummyUserOfficerWithRole,
-  dummyUserWithRole,
 } from '../datasources/mockups/UserDataSource';
 import ReviewQueries from './ReviewQueries';
 
@@ -18,7 +18,7 @@ test('A userofficer can get a review', () => {
 
 test('A user can not get a review', () => {
   return expect(
-    reviewQueries.get(dummyUserWithRole, { reviewId: 1 })
+    reviewQueries.get(dummyUserNotOnProposalWithRole, { reviewId: 1 })
   ).resolves.toBe(null);
 });
 
@@ -32,6 +32,8 @@ test('A userofficer can get reviews for a proposal', () => {
 
 test('A user can not get reviews for a proposal', () => {
   return expect(
-    reviewQueries.reviewsForProposal(dummyUserWithRole, { proposalPk: 10 })
+    reviewQueries.reviewsForProposal(dummyUserNotOnProposalWithRole, {
+      proposalPk: 10,
+    })
   ).resolves.toStrictEqual(null);
 });

apps/backend/src/resolvers/types/User.ts

@@ -136,13 +136,23 @@ export class UserResolver {
 
     const shouldGetOnlyUserReviews = !reviewer;
 
-    return context.queries.review.dataSource.getUserReviews(
-      fapsUserIsMemberOf.map((faps) => faps.id),
-      shouldGetOnlyUserReviews ? user.id : undefined,
-      callId,
-      instrumentId,
-      status
-    );
+    if (shouldGetOnlyUserReviews) {
+      return context.queries.review.dataSource.getUserReviews(
+        fapsUserIsMemberOf.map((faps) => faps.id),
+        user.id,
+        callId,
+        instrumentId,
+        status
+      );
+    } else {
+      return context.queries.review.dataSource.getAllUsersReviews(
+        fapsUserIsMemberOf.map((faps) => faps.id),
+        user.id,
+        callId,
+        instrumentId,
+        status
+      );
+    }
   }
 
   @FieldResolver(() => [Proposal])

apps/e2e/cypress/e2e/FAPs.cy.ts

@@ -156,6 +156,10 @@ const proposal2 = {
   title: faker.random.words(3),
   abstract: faker.random.words(5),
 };
+const proposal3 = {
+  title: faker.random.words(3),
+  abstract: faker.random.words(5),
+};
 
 const scientist = initialDBData.users.user1;
 
@@ -185,6 +189,7 @@ let createdCallId: number;
 let firstCreatedProposalPk: number;
 let firstCreatedProposalId: string;
 let secondCreatedProposalPk: number;
+let thirdCreatedProposalPk: number;
 let createdWorkflowId: number;
 let createdEsiTemplateId: number;
 let newlyCreatedInstrumentId: number;
@@ -1229,15 +1234,7 @@ context('Fap reviews tests', () => {
       });
       cy.assignReviewersToFap({
         fapId: createdFapId,
-        memberIds: [fapMembers.reviewer.id],
-      });
-
-      cy.assignFapReviewersToProposals({
-        assignments: {
-          memberId: fapMembers.reviewer2.id,
-          proposalPk: firstCreatedProposalPk,
-        },
-        fapId: createdFapId,
+        memberIds: [fapMembers.reviewer.id, fapMembers.reviewer2.id],
       });
 
       cy.assignFapReviewersToProposals({
@@ -1295,6 +1292,39 @@ context('Fap reviews tests', () => {
           });
         }
       });
+
+      cy.createProposal({ callId: initialDBData.call.id }).then((result) => {
+        const createdProposal = result.createProposal;
+        if (createdProposal) {
+          thirdCreatedProposalPk = createdProposal.primaryKey;
+
+          cy.updateProposal({
+            proposalPk: thirdCreatedProposalPk,
+            title: proposal3.title,
+            abstract: proposal3.abstract,
+            proposerId: initialDBData.users.user1.id,
+          });
+
+          cy.assignProposalsToInstruments({
+            instrumentIds: [newlyCreatedInstrumentId],
+            proposalPks: [thirdCreatedProposalPk],
+          });
+          cy.assignProposalsToFaps({
+            fapInstruments: [
+              { instrumentId: newlyCreatedInstrumentId, fapId: createdFapId },
+            ],
+            proposalPks: [thirdCreatedProposalPk],
+          });
+
+          cy.assignFapReviewersToProposals({
+            assignments: {
+              memberId: fapMembers.reviewer2.id,
+              proposalPk: thirdCreatedProposalPk,
+            },
+            fapId: createdFapId,
+          });
+        }
+      });
       cy.login(fapMembers.reviewer);
 
       cy.changeActiveRole(initialDBData.roles.fapReviewer);
@@ -1377,6 +1407,45 @@ context('Fap reviews tests', () => {
         variant: 'success',
       });
     });
+    it('Fap Reviewer should only see their proposals and all proposals when selecting My Proposals and All proposals respectively from reviewer dropdown', () => {
+      cy.get('#reviewer-selection', { timeout: 5000 })
+        .parent()
+        .should('be.visible')
+        .click();
+      cy.get('[role="presentation"]').contains('My proposals').click();
+      cy.finishedLoading();
+      cy.contains(proposal1.title);
+      cy.contains(proposal3.title).should('not.exist');
+
+      cy.get('#reviewer-selection', { timeout: 5000 })
+        .parent()
+        .should('be.visible')
+        .click();
+      cy.get('[role="presentation"]').contains('All proposals').click();
+      cy.contains(proposal1.title);
+      cy.contains(proposal3.title);
+    });
+    it('Fap Reviewer should not be able to submit a grade for proposals on which they are not reviewer, they should only able to view them', () => {
+      cy.get('#reviewer-selection', { timeout: 5000 })
+        .parent()
+        .should('be.visible')
+        .click();
+      cy.get('[role="presentation"]').contains('All proposals').click();
+
+      cy.finishedLoading();
+      cy.contains(proposal3.title).parent().contains('Draft');
+      cy.contains(proposal3.title)
+        .parent()
+        .find('[data-cy="view-proposal-details-icon"]')
+        .should('be.visible');
+      cy.contains(proposal3.title)
+        .parent()
+        .find('[data-cy="view-proposal-details-icon"]')
+        .click();
+      cy.get('[role="presentation"] [role="tab"]').contains('Grade').click();
+      cy.contains('button', 'Review').click();
+      cy.get('[data-cy="button-submit-proposal"]').should('be.disabled');
+    });
 
     it('FAP review should be removed if proposal is removed from instrument', () => {
       cy.contains(proposal1.title);
@@ -1422,27 +1491,11 @@ context('Fap reviews tests', () => {
         .find('[data-cy="grade-proposal-icon"]')
         .click();
 
-      cy.getProposalReviews({
-        proposalPk: firstCreatedProposalPk,
-      }).then(({ proposalReviews }) => {
-        if (proposalReviews) {
-          cy.updateReview({
-            reviewID: proposalReviews[0].id,
-            comment: faker.random.words(5),
-            grade: 5,
-            status: ReviewStatus.SUBMITTED,
-            fapID: createdFapId,
-            questionaryID: proposalReviews[0].questionaryID,
-          });
-        }
-      });
-
       readWriteReview({ shouldSubmit: true, isReviewer: true });
 
       cy.finishedLoading();
 
       cy.contains(fapMembers.reviewer.lastName).parent().contains('SUBMITTED');
-      cy.contains(fapMembers.reviewer2.lastName).parent().contains('SUBMITTED');
     });
   });
 });

apps/e2e/cypress/fixtures/exampleCallFapExport.json

@@ -1,10 +1,10 @@
 [
-  { "Proposal Reference Number": "local lumen" },
+  { "Proposal Reference Number": "Central lavender" },
   {
     "Proposal Reference Number": "567122",
     "Proposal Title": "lumen proofread hertz",
     "Principal Investigator": "Carl Carlsson",
-    "Instrument": "local lumen",
+    "Instrument": "Central lavender",
     "Instrument available time": 20,
     "Technical review allocated time": 25,
     "Fap allocated time": 25,
@@ -20,7 +20,7 @@
     "Proposal Reference Number": "701367",
     "Proposal Title": "web Connecticut driver",
     "Principal Investigator": "Carl Carlsson",
-    "Instrument": "local lumen",
+    "Instrument": "Central lavender",
     "Instrument available time": 20,
     "Technical review allocated time": 5,
     "Fap allocated time": 5,

apps/frontend/src/components/review/ProposalGrade.tsx

@@ -57,7 +57,6 @@ const ProposalGrade = ({
   const [numberOfChars, setNumberOfChars] = useState(0);
   const hasAccessRights = useCheckAccess([UserRole.USER_OFFICER]);
   const { settingsMap } = useContext(SettingsContext);
-
   const gradeDecimalPoints = parseFloat(
     settingsMap.get(SettingsId.GRADE_PRECISION)?.settingsValue?.valueOf() ?? '1'
   );