feat: Track vendor approvals across version updates through result co…

…py process (#1267) * Tweaks to address visual glitch when clicking last Next button * Add 'nvAccess' vendor seeder * Update signMeInAsVendor function and instructions to support signing in as a specific vendor * Preserve vendor approval status across result copy process * Update snapshots * Prevent phase update to candidate from overwriting older result's vendor approval status * Update tests to verify vendor approval being copied
w3c · Dec 5, 2024 · 90807df · 90807df
1 parent 0fe3946
commit 90807df
Show file tree

Hide file tree

Showing 12 changed files with 499 additions and 44 deletions.
diff --git a/client/components/CandidateReview/CandidateTestPlanRun/index.jsx b/client/components/CandidateReview/CandidateTestPlanRun/index.jsx
@@ -121,7 +121,7 @@ const CandidateTestPlanRun = () => {
       setFirstTimeViewing(true);
       setViewedTests(tests => [...tests, currentTest.id]);
       await addViewerToTest(currentTest.id);
-      refetch();
+      await refetch();
     } else {
       setFirstTimeViewing(false);
     }
@@ -151,7 +151,7 @@ const CandidateTestPlanRun = () => {
 
   const submitApproval = async (status = '') => {
     if (status === 'APPROVED') {
-      updateVendorStatus(true);
+      await updateVendorStatus(true);
       setConfirmationModal(
         <ApprovedModal
           handleAction={async () => {
@@ -180,8 +180,9 @@ const CandidateTestPlanRun = () => {
       if (
         !tests[0].viewers?.find(viewer => viewer.username === data.me.username)
       ) {
-        addViewerToTest(tests[0].id);
-        setFirstTimeViewing(true);
+        addViewerToTest(tests[0].id).then(() => {
+          setFirstTimeViewing(true);
+        });
       }
       const viewedTests = [
         tests[0].id,
@@ -212,23 +213,17 @@ const CandidateTestPlanRun = () => {
   useEffect(() => {
     if (data) {
       updateVendorStatus();
-    }
-  }, [reviewStatus]);
-
-  useEffect(() => {
-    if (data) {
       updateTestViewed();
+      if (currentTestIndex !== 0) setIsFirstTest(false);
+      if (tests?.length === 1) setIsLastTest(true);
+      if (currentTestIndex + 1 === tests?.length) setIsLastTest(true);
     }
-  }, [currentTestIndex]);
-
-  useEffect(() => {
-    if (data) {
-      setIsLastTest(tests?.length === 1);
-    }
-  }, [data, tests]);
+  }, [reviewStatus, currentTestIndex]);
 
   useEffect(() => {
-    if (isLastTest) finishButtonRef.current.focus();
+    // Prevent a plan with only 1 test from immediately pushing the focus to the
+    // submit button
+    if (isLastTest && tests?.length !== 1) finishButtonRef.current.focus();
   }, [isLastTest]);
 
   if (error)
@@ -377,9 +372,9 @@ const CandidateTestPlanRun = () => {
     NVDA: 'nvda'
   };
 
-  if (githubAtLabelMap[at] == 'vo') {
+  if (githubAtLabelMap[at] === 'vo') {
     fileBugUrl = 'https://bugs.webkit.org/buglist.cgi?quicksearch=voiceover';
-  } else if (githubAtLabelMap[at] == 'nvda') {
+  } else if (githubAtLabelMap[at] === 'nvda') {
     fileBugUrl = 'https://github.com/nvaccess/nvda/issues';
   } else {
     fileBugUrl =
@@ -667,14 +662,14 @@ const CandidateTestPlanRun = () => {
             issue =>
               issue.isCandidateReview &&
               issue.feedbackType === 'FEEDBACK' &&
-              issue.author == data.me.username
+              issue.author === data.me.username
           )}
           feedbackGithubUrl={feedbackGithubUrl}
           changesRequestedIssues={testPlanReport.issues?.filter(
             issue =>
               issue.isCandidateReview &&
               issue.feedbackType === 'CHANGES_REQUESTED' &&
-              issue.author == data.me.username
+              issue.author === data.me.username
           )}
           changesRequestedGithubUrl={changesRequestedGithubUrl}
           handleAction={submitApproval}

diff --git a/client/index.js b/client/index.js
@@ -50,20 +50,57 @@ window.signMeInAsTester = username => {
   return signMeInCommon({ username, roles: [{ name: 'TESTER' }] });
 };
 
-window.signMeInAsVendor = username => {
+/**
+ * @param {string} username
+ * @param {'vispero'|'nvAccess'|'apple'} vendor
+ * @returns {Promise<void>}
+ */
+window.signMeInAsVendor = (username, vendor = 'vispero') => {
+  let company = {
+    id: '1',
+    name: 'vispero',
+    ats: [
+      {
+        id: '1',
+        name: 'JAWS'
+      }
+    ]
+  };
+
+  switch (vendor.trim()) {
+    case 'nvAccess':
+      company = {
+        id: '6',
+        name: 'nvAccess',
+        ats: [
+          {
+            id: '2',
+            name: 'NVDA'
+          }
+        ]
+      };
+      break;
+    case 'apple':
+      company = {
+        id: '4',
+        name: 'apple',
+        ats: [
+          {
+            id: '3',
+            name: 'VoiceOver for macOS'
+          }
+        ]
+      };
+      break;
+    default:
+      // maintain default vispero selection
+      break;
+  }
+
   return signMeInCommon({
     username,
     roles: [{ name: 'VENDOR' }],
-    company: {
-      id: '1',
-      name: 'vispero',
-      ats: [
-        {
-          id: '1',
-          name: 'JAWS'
-        }
-      ]
-    }
+    company
   });
 };
 

diff --git a/docs/local-development.md b/docs/local-development.md
@@ -46,6 +46,14 @@ Another way to log in as either a tester or admin, useful for quick testing and
         ```
           signMeInAsVendor("joe-the-vendor")
         ```
+      By default, you will be signed in as a "vispero (JAWS)" vendor but to sign in as any others, you can provide the vendor's company name as the second parameter. eg:
+        ```
+          signMeInAsVendor("joe-the-vendor", "apple")
+        ```
+      The list of applicable constants is currently:
+        1. vispero (JAWS)
+        2. nvAccess (NVDA)
+        3. apple (VoiceOver for macOS)
 
 The part in quotes is the username, feel free to change the username to whatever you prefer.
 

diff --git a/server/models/services/TestPlanVersionService.js b/server/models/services/TestPlanVersionService.js
@@ -328,10 +328,42 @@ const updateTestPlanVersionById = async ({
   });
 };
 
+/**
+ * @param {object} options
+ * @param {number} id - id of the TestPlanVersion record to be updated
+ * @param {number} testId
+ * @param {object[]} viewers
+ * @param {*} options.transaction - Sequelize transaction
+ * @returns {Promise<void>}
+ */
+const updateTestViewersOnTestPlanVersion = async ({
+  id,
+  testId,
+  viewers = [],
+  transaction
+}) => {
+  await ModelService.rawQuery(
+    `
+      update "TestPlanVersion"
+      set tests = ( select jsonb_agg(case when element ->> 'id' = '${testId}'
+                                            then jsonb_set(element, '{viewers}', '${JSON.stringify(
+                                              viewers
+                                            )}'::jsonb) else element end)
+                    from jsonb_array_elements(tests) as element )
+      where id = ${id}
+        and tests @> '[{"id": "${testId}"}]';
+    `,
+    { transaction }
+  );
+};
+
 module.exports = {
   // Basic CRUD
   getTestPlanVersionById,
   getTestPlanVersions,
   createTestPlanVersion,
-  updateTestPlanVersionById
+  updateTestPlanVersionById,
+
+  // Custom functions
+  updateTestViewersOnTestPlanVersion
 };
diff --git a/server/resolvers/TestPlanReportOperations/promoteVendorReviewStatusResolver.js b/server/resolvers/TestPlanReportOperations/promoteVendorReviewStatusResolver.js
@@ -1,14 +1,22 @@
+const { AuthenticationError } = require('apollo-server-errors');
 const {
   updateTestPlanReportById
 } = require('../../models/services/TestPlanReportService');
 const populateData = require('../../services/PopulatedData/populateData');
+const checkUserRole = require('../helpers/checkUserRole');
 
 const promoteVendorReviewStatusResolver = async (
   { parentContext: { id: testPlanReportId } },
   { vendorReviewStatus },
   context
 ) => {
-  const { transaction } = context;
+  const { user, transaction } = context;
+
+  const isAdmin = checkUserRole.isAdmin(user?.roles);
+  const isVendor = checkUserRole.isVendor(user?.roles);
+  if (!(isAdmin || isVendor)) {
+    throw new AuthenticationError();
+  }
 
   let values = { vendorReviewStatus };
 

diff --git a/server/resolvers/TestPlanVersionOperations/updatePhaseResolver.js b/server/resolvers/TestPlanVersionOperations/updatePhaseResolver.js
@@ -225,9 +225,13 @@ const updatePhaseResolver = async (
       if (phase === 'CANDIDATE') {
         updateParams = {
           ...updateParams,
-          metrics: { ...testPlanReport.metrics, ...metrics },
-          vendorReviewStatus: 'READY'
+          metrics: { ...testPlanReport.metrics, ...metrics }
         };
+
+        // In the instance where an older result's status is being copied over
+        if (!testPlanReport.vendorReviewStatus) {
+          updateParams = { ...updateParams, vendorReviewStatus: 'READY' };
+        }
       } else if (phase === 'RECOMMENDED') {
         updateParams = {
           ...updateParams,

diff --git a/server/resolvers/addViewerResolver.js b/server/resolvers/addViewerResolver.js
@@ -3,16 +3,14 @@ const {
   getTestPlanVersionById,
   updateTestPlanVersionById
 } = require('../models/services/TestPlanVersionService');
+const checkUserRole = require('./helpers/checkUserRole');
 
 const addViewerResolver = async (_, { testPlanVersionId, testId }, context) => {
   const { user, transaction } = context;
 
-  if (
-    !(
-      user?.roles.find(role => role.name === 'ADMIN') ||
-      user?.roles.find(role => role.name === 'VENDOR')
-    )
-  ) {
+  const isAdmin = checkUserRole.isAdmin(user?.roles);
+  const isVendor = checkUserRole.isVendor(user?.roles);
+  if (!(isAdmin || isVendor)) {
     throw new AuthenticationError();
   }
 

diff --git a/server/resolvers/helpers/checkUserRole.js b/server/resolvers/helpers/checkUserRole.js
@@ -0,0 +1,15 @@
+const ROLES = {
+  TESTER: 'TESTER',
+  ADMIN: 'ADMIN',
+  VENDOR: 'VENDOR'
+};
+
+const isTester = (roles = []) => roles.find(role => role.name === ROLES.TESTER);
+const isVendor = (roles = []) => roles.find(role => role.name === ROLES.VENDOR);
+const isAdmin = (roles = []) => roles.find(role => role.name === ROLES.ADMIN);
+
+module.exports = {
+  isTester,
+  isVendor,
+  isAdmin
+};
diff --git a/server/resolvers/helpers/processCopiedReports.js b/server/resolvers/helpers/processCopiedReports.js
@@ -1,5 +1,6 @@
 const {
-  getTestPlanVersionById
+  getTestPlanVersionById,
+  updateTestViewersOnTestPlanVersion
 } = require('../../models/services/TestPlanVersionService');
 const {
   getTestPlanReports,
@@ -328,6 +329,9 @@ const processCopiedReports = async ({
     );
 
     for (const oldTestPlanRun of oldTestPlanReport.testPlanRuns) {
+      const oldTestPlanRunVendorReviewStatus =
+        oldTestPlanReport.vendorReviewStatus;
+
       // Track which old test results need to be preserved
       const keptTestResultsByTestId = getKeptTestResultsByTestId(
         oldTestPlanRun.testResults,
@@ -358,6 +362,7 @@ const processCopiedReports = async ({
         },
         transaction
       });
+      let allResultsPreserved = true;
       const newTestResults = [];
 
       for (const testResultToSaveTestId of Object.keys(
@@ -375,6 +380,10 @@ const processCopiedReports = async ({
           { testId: testResultToSaveTestId },
           { context }
         );
+        const { test: oldTest } = await populateData(
+          { testResultId: oldTestResult.id },
+          { context }
+        );
 
         // Re-run createTestResultSkeleton to avoid unexpected scenario index matching issues when saving
         // future results; override newly generated test results with old results if exists
@@ -403,6 +412,7 @@ const processCopiedReports = async ({
           // Unknown combination of command + settings when compared with last version
           const oldScenarioResult = scenarioResultsByScenarioIds[rawScenarioId];
           if (!oldScenarioResult) {
+            allResultsPreserved = false;
             newTestResult.completedAt = null;
             continue;
           }
@@ -428,17 +438,43 @@ const processCopiedReports = async ({
             const oldAssertionResult =
               assertionResultsByAssertionIds[rawAssertionId];
             if (!oldAssertionResult) {
+              allResultsPreserved = false;
               newTestResult.completedAt = null;
               continue;
             }
 
+            // Update TestPlanVersion.tests to include the viewers from the old
+            // TestPlanVersion.tests
+            // TODO: Move viewers to TestPlanReport; more appropriate and
+            //  understandable database structure
+            if (oldTest.viewers) {
+              await updateTestViewersOnTestPlanVersion({
+                id: newTestPlanVersionId,
+                testId: testResultToSaveTestId,
+                viewers: oldTest.viewers,
+                transaction
+              });
+            }
+
             eachAssertionResult.passed = oldAssertionResult.passed;
           }
         }
 
         newTestResults.push(newTestResult);
       }
 
+      // Since no substantive changes, preserve the TestPlanReport's
+      // vendorReviewStatus if exists
+      if (allResultsPreserved && oldTestPlanRunVendorReviewStatus) {
+        await updateTestPlanReportById({
+          id: newTestPlanReport.id,
+          values: {
+            vendorReviewStatus: oldTestPlanRunVendorReviewStatus
+          },
+          transaction
+        });
+      }
+
       // Run updated metrics calculations for new TestPlanRun test results to be used in metrics calculations
       await updateMetricsAndMarkedFinalAtForTestPlanReport({
         newTestPlanReport,

diff --git a/server/resources/ats.json b/server/resources/ats.json
@@ -32,7 +32,7 @@
         "id": 2,
         "name": "NVDA",
         "key": "nvda",
-        "vendorId": null,
+        "vendorId": 6,
         "defaultConfigurationInstructionsHTML": "Configure NVDA with default settings. For help, read &lt;a href=&quot;https://github.com/w3c/aria-at/wiki/Configuring-Screen-Readers-for-Testing&quot;&gt;Configuring Screen Readers for Testing&lt;/a&gt;.",
         "assertionTokens": {
             "screenReader": "NVDA",