Merge pull request #1691 from gchq/bugfix/BAI-1530-error-handling-for…

…-model-exports restructured the model mirror page and added a backend check for exis…
gchq · Jan 8, 2025 · d8f045c · d8f045c
2 parents b763203 + 7927f02
commit d8f045c
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 58 deletions.
diff --git a/backend/src/services/mirroredModel.ts b/backend/src/services/mirroredModel.ts
@@ -46,6 +46,9 @@ export async function exportModel(
   if (!model.settings.mirror.destinationModelId) {
     throw BadReq(`The 'Destination Model ID' has not been set on this model.`)
   }
+  if (!model.card || !model.card.schemaId) {
+    throw BadReq('You must select a schema for your model before you can start the export process.')
+  }
   const mirroredModelId = model.settings.mirror.destinationModelId
   const auth = await authorisation.model(user, model, ModelAction.Update)
   if (!auth.success) {

diff --git a/backend/test/services/mirroredModel.spec.ts b/backend/test/services/mirroredModel.spec.ts
@@ -91,7 +91,7 @@ vi.mock('../../src/services/log.js', async () => ({
 }))
 
 const modelMocks = vi.hoisted(() => ({
-  getModelById: vi.fn(() => ({ settings: { mirror: { destinationModelId: '123' } } })),
+  getModelById: vi.fn(() => ({ settings: { mirror: { destinationModelId: '123' } }, card: { schemaId: 'test' } })),
   getModelCardRevisions: vi.fn(() => [{ toJSON: vi.fn(), version: 123 }]),
   setLatestImportedModelCard: vi.fn(),
   saveImportedModelCard: vi.fn(),
@@ -237,6 +237,7 @@ describe('services > mirroredModel', () => {
   test('exportModel > missing mirrored model ID', async () => {
     modelMocks.getModelById.mockReturnValueOnce({
       settings: { mirror: { destinationModelId: '' } },
+      card: { schemaId: 'test' },
     })
     const response = exportModel({} as UserInterface, 'modelId', true, ['1.2.3'])
     await expect(response).rejects.toThrowError(/^The 'Destination Model ID' has not been set on this model./)

diff --git a/frontend/src/entry/model/mirroredModels/ExportModelAgreement.tsx b/frontend/src/entry/model/mirroredModels/ExportModelAgreement.tsx
@@ -1,5 +1,5 @@
 import { LoadingButton } from '@mui/lab'
-import { Box, Button, Card, Checkbox, Container, FormControlLabel, Stack, Typography } from '@mui/material'
+import { Box, Button, Checkbox, FormControlLabel, Stack, Typography } from '@mui/material'
 import { postModelExportToS3 } from 'actions/model'
 import { ChangeEvent, FormEvent, useState } from 'react'
 import Restricted from 'src/common/Restricted'
@@ -46,27 +46,25 @@ export default function ExportModelAgreement({ modelId }: ExportModelAgreementPr
   }
 
   return (
-    <Container maxWidth='md'>
-      <Card sx={{ mx: 'auto', my: 4, p: 4 }}>
-        <Typography variant='h6' component='h1' color='primary' align='center'>
-          Model Export Agreement
-        </Typography>
-        <Box component='form' onSubmit={handleSubmit}>
-          <Stack spacing={2} alignItems='start' justifyContent='start'>
-            <ModelExportAgreementText />
-            <FormControlLabel
-              control={<Checkbox checked={checked} onChange={handleChecked} />}
-              label='I agree to the terms and conditions of this model export agreement'
-            />
-            <Restricted action='exportMirroredModel' fallback={<Button disabled>Submit</Button>}>
-              <LoadingButton variant='contained' loading={loading} disabled={!checked} type='submit'>
-                Submit
-              </LoadingButton>
-            </Restricted>
-            <MessageAlert message={errorMessage} severity='error' />
-          </Stack>
-        </Box>
-      </Card>
-    </Container>
+    <>
+      <Typography variant='h6' component='h1' color='primary'>
+        Request a Model Export
+      </Typography>
+      <Box component='form' onSubmit={handleSubmit}>
+        <Stack spacing={2} alignItems='start' justifyContent='start'>
+          <ModelExportAgreementText />
+          <FormControlLabel
+            control={<Checkbox checked={checked} onChange={handleChecked} />}
+            label='I agree to the terms and conditions of this model export agreement'
+          />
+          <Restricted action='exportMirroredModel' fallback={<Button disabled>Submit</Button>}>
+            <LoadingButton variant='contained' loading={loading} disabled={!checked} type='submit'>
+              Submit
+            </LoadingButton>
+          </Restricted>
+          <MessageAlert message={errorMessage} severity='error' />
+        </Stack>
+      </Box>
+    </>
   )
 }
diff --git a/frontend/src/entry/model/mirroredModels/ExportSettings.tsx b/frontend/src/entry/model/mirroredModels/ExportSettings.tsx
@@ -1,6 +1,17 @@
 import ExpandMoreIcon from '@mui/icons-material/ExpandMore'
 import { LoadingButton } from '@mui/lab'
-import { Accordion, AccordionDetails, AccordionSummary, Box, Stack, TextField, Typography } from '@mui/material'
+import {
+  Accordion,
+  AccordionDetails,
+  AccordionSummary,
+  Box,
+  Card,
+  Container,
+  Divider,
+  Stack,
+  TextField,
+  Typography,
+} from '@mui/material'
 import { patchModel } from 'actions/model'
 import { ChangeEvent, FormEvent, useState } from 'react'
 import LabelledInput from 'src/common/LabelledInput'
@@ -19,6 +30,7 @@ export default function ExportSettings({ model }: ExportSettingsProps) {
   const [destinationModelId, setDestinationModelId] = useState(model.settings.mirror?.destinationModelId || '')
   const [loading, setLoading] = useState(false)
   const [errorMessage, setErrorMessage] = useState('')
+  const [isSettingsOpen, setIsSettingsOpen] = useState(destinationModelId === undefined || destinationModelId === '')
 
   // TODO - Add the ability to filter releases needed for export (This functionality is not available on the backend)
   // const [selectedReleases, setSelectedReleases] = useState<ReleaseInterface[]>([])
@@ -55,25 +67,31 @@ export default function ExportSettings({ model }: ExportSettingsProps) {
 
   return (
     <>
-      <ExportModelAgreement modelId={model.id} />
-      <Accordion sx={{ borderTop: 'none' }} slotProps={{ heading: { component: 'h3' } }}>
-        <AccordionSummary sx={{ pl: 0 }} expandIcon={<ExpandMoreIcon />}>
-          <Typography sx={{ width: '100%' }} color='primary' variant='h6' component='div'>
-            Export Settings
-          </Typography>
-        </AccordionSummary>
-        <AccordionDetails>
+      <Container maxWidth='md'>
+        <Card sx={{ mx: 'auto', my: 4, p: 4 }}>
           <Box component='form' onSubmit={handleSave}>
-            <Stack spacing={2} alignItems='flex-start'>
-              <LabelledInput label={'Destination Model ID'} htmlFor={'destination-model-id'} required>
-                <TextField
-                  id='destination-model-id'
-                  value={destinationModelId}
-                  onChange={handleDestinationModelId}
-                  size='small'
-                />
-              </LabelledInput>
-              {/*TODO - Add the ability to filter releases needed for export (This functionality is not available on the backend)
+            <Stack spacing={3} divider={<Divider flexItem />}>
+              <Accordion
+                expanded={isSettingsOpen}
+                onChange={() => setIsSettingsOpen(isSettingsOpen ? false : true)}
+                slotProps={{ heading: { component: 'h3' } }}
+              >
+                <AccordionSummary expandIcon={<ExpandMoreIcon />} sx={{ px: 0 }}>
+                  <Typography sx={{ width: '100%' }} color='primary' variant='h6' component='div'>
+                    Model Export Settings
+                  </Typography>
+                </AccordionSummary>
+                <AccordionDetails>
+                  <Stack spacing={2}>
+                    <LabelledInput label={'Destination Model ID'} htmlFor={'destination-model-id'} required>
+                      <TextField
+                        id='destination-model-id'
+                        value={destinationModelId}
+                        onChange={handleDestinationModelId}
+                        size='small'
+                      />
+                    </LabelledInput>
+                    {/*TODO - Add the ability to filter releases needed for export (This functionality is not available on the backend)
               <ReleaseSelector
                 model={model}
                 selectedReleases={selectedReleases}
@@ -82,20 +100,24 @@ export default function ExportSettings({ model }: ExportSettingsProps) {
                 requiredRolesText={requiredRolesText}
               />
                */}
-              <LoadingButton
-                sx={{ width: 'fit-content' }}
-                variant='contained'
-                data-test='createAccessRequestButton'
-                loading={loading}
-                type='submit'
-              >
-                Save
-              </LoadingButton>
-              <MessageAlert message={errorMessage} severity='error' />
+                    <LoadingButton
+                      sx={{ width: 'fit-content' }}
+                      variant='contained'
+                      data-test='createAccessRequestButton'
+                      loading={loading}
+                      type='submit'
+                    >
+                      Save
+                    </LoadingButton>
+                    <MessageAlert message={errorMessage} severity='error' />
+                  </Stack>
+                </AccordionDetails>
+              </Accordion>
+              <ExportModelAgreement modelId={model.id} />
             </Stack>
           </Box>
-        </AccordionDetails>
-      </Accordion>
+        </Card>
+      </Container>
     </>
   )
 }
diff --git a/frontend/src/entry/model/reviews/ApprovalsDisplay.tsx b/frontend/src/entry/model/reviews/ApprovalsDisplay.tsx
@@ -1,8 +1,7 @@
-import { EmotionJSX } from '@emotion/react/types/jsx-namespace'
 import { Done } from '@mui/icons-material'
 import { Stack, Tooltip, Typography } from '@mui/material'
 import { useGetModelRoles } from 'actions/model'
-import { useMemo } from 'react'
+import { ReactElement, useMemo } from 'react'
 import Loading from 'src/common/Loading'
 import MessageAlert from 'src/MessageAlert'
 import { ResponseInterface } from 'types/types'
@@ -22,7 +21,7 @@ export default function ApprovalsDisplay({ modelId, acceptedReviewResponses }: A
 
   const roleApprovals = useMemo(
     () =>
-      dynamicRoles.reduce<EmotionJSX.Element[]>((approvals, dynamicRole) => {
+      dynamicRoles.reduce<ReactElement[]>((approvals, dynamicRole) => {
         const hasRoleApproved = !!acceptedReviewResponses.find(
           (acceptedResponse) => acceptedResponse.role === dynamicRole.id,
         )