Address code review comments + more component fixes

Author: NatSquared

api/src/api/models/submission.py

@@ -4,7 +4,7 @@
 """
 from __future__ import annotations
 
-from typing import List
+from typing import List, Optional
 
 from sqlalchemy import ForeignKey
 from sqlalchemy.dialects import postgresql
@@ -26,20 +26,27 @@ class Submission(BaseModel):  # pylint: disable=too-few-public-methods
     __tablename__ = 'submission'
 
     id = db.Column(db.Integer, primary_key=True, autoincrement=True)
-    submission_json = db.Column(postgresql.JSONB(astext_type=db.Text()), nullable=False, server_default='{}')
-    survey_id = db.Column(db.Integer, ForeignKey('survey.id', ondelete='CASCADE'), nullable=False)
-    engagement_id = db.Column(db.Integer, ForeignKey('engagement.id', ondelete='CASCADE'), nullable=False)
-    participant_id = db.Column(db.Integer, ForeignKey('participant.id'), nullable=True)
+    submission_json = db.Column(postgresql.JSONB(
+        astext_type=db.Text()), nullable=False, server_default='{}')
+    survey_id = db.Column(db.Integer, ForeignKey(
+        'survey.id', ondelete='CASCADE'), nullable=False)
+    engagement_id = db.Column(db.Integer, ForeignKey(
+        'engagement.id', ondelete='CASCADE'), nullable=False)
+    participant_id = db.Column(
+        db.Integer, ForeignKey('participant.id'), nullable=True)
     reviewed_by = db.Column(db.String(50))
     review_date = db.Column(db.DateTime)
-    comment_status_id = db.Column(db.Integer, ForeignKey('comment_status.id', ondelete='SET NULL'))
+    comment_status_id = db.Column(db.Integer, ForeignKey(
+        'comment_status.id', ondelete='SET NULL'))
     has_personal_info = db.Column(db.Boolean, nullable=True)
     has_profanity = db.Column(db.Boolean, nullable=True)
     rejected_reason_other = db.Column(db.String(500), nullable=True)
     has_threat = db.Column(db.Boolean, nullable=True)
     notify_email = db.Column(db.Boolean(), default=True)
-    comments = db.relationship('Comment', backref='submission', cascade='all, delete')
-    staff_note = db.relationship('StaffNote', backref='submission', cascade='all, delete')
+    comments = db.relationship(
+        'Comment', backref='submission', cascade='all, delete')
+    staff_note = db.relationship(
+        'StaffNote', backref='submission', cascade='all, delete')
 
     @classmethod
     def get_by_survey_id(cls, survey_id) -> List[SubmissionSchema]:
@@ -118,7 +125,7 @@ def update(cls, submission: SubmissionSchema, session=None) -> Submission:
         return query.first()
 
     @classmethod
-    def update_comment_status(cls, submission_id, comment: dict, session=None) -> Submission:
+    def update_comment_status(cls, submission_id, comment: dict, session=None) -> Optional[Submission]:
         """Update comment status."""
         status_id = comment.get('status_id', None)
         has_personal_info = comment.get('has_personal_info', None)

api/src/api/resources/survey.py

@@ -87,14 +87,21 @@ def get():
                 exclude_hidden=args.get('exclude_hidden', False, bool),
                 exclude_template=args.get('exclude_template', False, bool),
                 search_text=args.get('search_text', '', str),
-                is_unlinked=args.get('is_unlinked', default=False, type=lambda v: v.lower() == 'true'),
-                is_linked=args.get('is_linked', default=False, type=lambda v: v.lower() == 'true'),
-                is_hidden=args.get('is_hidden', default=False, type=lambda v: v.lower() == 'true'),
-                is_template=args.get('is_template', default=False, type=lambda v: v.lower() == 'true'),
-                created_date_from=args.get('created_date_from', None, type=str),
+                is_unlinked=args.get(
+                    'is_unlinked', default=False, type=lambda v: v.lower() == 'true'),
+                is_linked=args.get('is_linked', default=False,
+                                   type=lambda v: v.lower() == 'true'),
+                is_hidden=args.get('is_hidden', default=False,
+                                   type=lambda v: v.lower() == 'true'),
+                is_template=args.get(
+                    'is_template', default=False, type=lambda v: v.lower() == 'true'),
+                created_date_from=args.get(
+                    'created_date_from', None, type=str),
                 created_date_to=args.get('created_date_to', None, type=str),
-                published_date_from=args.get('published_date_from', None, type=str),
-                published_date_to=args.get('published_date_to', None, type=str),
+                published_date_from=args.get(
+                    'published_date_from', None, type=str),
+                published_date_to=args.get(
+                    'published_date_to', None, type=str),
             )
 
             survey_records = SurveyService()\

web/src/components/Form/MultiPageForm.tsx

@@ -18,7 +18,6 @@ const MultiPageForm = ({
 }: FormSubmitterProps) => {
     const [currentPage, setCurrentPage] = useState(0);
     const simpleFileOptions = createSimpleFileOptions({ verificationToken });
-    console.log(savedForm);
     const handleScrollUp = () => {
         globalThis.scrollTo({
             top: 100,

web/src/components/common/Navigation/Breadcrumb.tsx

@@ -1,5 +1,5 @@
 import { Breadcrumbs } from '@mui/material';
-import React, { useEffect, useMemo } from 'react';
+import React, { useMemo } from 'react';
 import { BodyText } from '../Typography';
 import { Link } from '.';
 import { UIMatch, useMatches } from 'react-router';
@@ -81,7 +81,6 @@ export interface UIMatchWithCrumb extends UIMatch<unknown, UIRouteHandle> {}
 export const AutoBreadcrumbs: React.FC<{ smallScreenOnly?: boolean }> = ({ smallScreenOnly }) => {
     const matches = (useMatches() as UIMatchWithCrumb[]).filter((match) => match.handle?.crumb);
     const matchKey = matches.map((m) => m.pathname).join('|');
-    const [resolvedCrumbs, setResolvedCrumbs] = React.useState<Record<string, BreadcrumbProps>>({});
 
     const crumbs = useMemo(() => {
         return matches.map((match) => {
@@ -96,40 +95,20 @@ export const AutoBreadcrumbs: React.FC<{ smallScreenOnly?: boolean }> = ({ small
         });
     }, [matchKey]); // Recompute only when matches change
 
-    useEffect(() => {
-        let cancelled = false;
-
-        const setNewCrumbs = (
-            resolvedCrumb: BreadcrumbProps,
-            previousCrumbs: Record<string, BreadcrumbProps>,
-            pathname: string,
-        ) => {
-            const previousCrumb = previousCrumbs[pathname];
-
-            // Avoid unnecessary re-renders if the crumb did not actually change.
-            if (previousCrumb?.name === resolvedCrumb?.name && previousCrumb?.link === resolvedCrumb?.link) {
-                return previousCrumbs;
-            }
-
-            return {
-                ...previousCrumbs,
-                [pathname]: resolvedCrumb,
-            };
-        };
-
-        crumbs.forEach(async (unresolvedCrumb, index) => {
-            const pathname = matches[index]?.pathname;
-            if (!pathname) return;
-
-            const resolvedCrumb = await unresolvedCrumb;
-            if (cancelled) return;
-            setResolvedCrumbs((previousCrumbs) => setNewCrumbs(resolvedCrumb, previousCrumbs, pathname));
-        });
-
-        return () => {
-            cancelled = true;
-        };
-    }, [crumbs, matches]);
+    const resolvedCrumbs = crumbs.map((unresolvedCrumb) => {
+        if (unresolvedCrumb instanceof Promise) {
+            return React.use(unresolvedCrumb);
+        }
+        return unresolvedCrumb;
+    });
+
+    const crumbMap = Object.create(null);
+    resolvedCrumbs.forEach((crumb, index) => {
+        const pathname = matches[index]?.pathname;
+        if (pathname) {
+            crumbMap[pathname] = crumb;
+        }
+    });
 
     return (
         <Breadcrumbs
@@ -140,7 +119,7 @@ export const AutoBreadcrumbs: React.FC<{ smallScreenOnly?: boolean }> = ({ small
             }}
         >
             {matches.map((match, index) => {
-                const resolvedCrumb = resolvedCrumbs[match.pathname];
+                const resolvedCrumb = crumbMap[match.pathname];
                 if (!resolvedCrumb) return null;
                 const name = resolvedCrumb?.name;
                 const link = index < matches.length - 1 ? (resolvedCrumb?.link ?? match.pathname) : undefined;

web/src/components/engagement/admin/EngagementLoaderAdmin.tsx

@@ -1,4 +1,4 @@
-import { redirect, Params } from 'react-router';
+import { redirect, LoaderFunctionArgs } from 'react-router';
 import { getAvailableTranslationLanguages, getEngagement, getEngagementBySlug } from 'services/engagementService';
 import { getWidgets } from 'services/widgetService';
 import { getEngagementMetadata, getMetadataTaxa } from 'services/engagementMetadataService';
@@ -23,7 +23,7 @@ export type EngagementLoaderAdminData = {
     hasDefaultLanguageTranslation: Promise<boolean>;
 };
 
-export const engagementLoaderAdmin = async ({ params }: { params: Params<string> }) => {
+export const engagementLoaderAdmin = async ({ params }: LoaderFunctionArgs) => {
     const { slug: slugParam, engagementId } = params;
     const defaultLanguageCode = AppConfig.language.defaultLanguageId.toLowerCase();
     const engagement = (slugParam ? getEngagementBySlug(slugParam) : getEngagement(Number(engagementId))).then(

web/src/components/engagement/admin/create/authoring/authoringLoader.tsx

@@ -2,7 +2,7 @@ import { Engagement } from 'models/engagement';
 import { EngagementDetailsTab } from 'models/engagementDetailsTab';
 import { SuggestedEngagement } from 'models/suggestedEngagement';
 import { Survey } from 'models/survey';
-import { Params } from 'react-router';
+import { LoaderFunctionArgs } from 'react-router';
 import { getDetailsTabs } from 'services/engagementDetailsTabService';
 import { getEngagement, getEngagements } from 'services/engagementService';
 import { getSurveysPage } from 'services/surveyService';
@@ -23,7 +23,7 @@ export type AuthoringLoaderData = {
     suggestions: Promise<SuggestedEngagement[]>;
 };
 
-const authoringLoader = async ({ params }: { params: Params<string> }) => {
+const authoringLoader = async ({ params }: LoaderFunctionArgs) => {
     const { engagementId, tenantId, languageCode } = params;
     const id = Number(engagementId);
     const tId = Number(tenantId);

web/src/components/engagement/preview/engagementPreviewLoader.tsx

@@ -1,4 +1,4 @@
-import { Params } from 'react-router';
+import { LoaderFunctionArgs } from 'react-router';
 import { getAvailableTranslationLanguages, getEngagement } from 'services/engagementService';
 import { getWidgets } from 'services/widgetService';
 import { getEngagementMetadata, getMetadataTaxa } from 'services/engagementMetadataService';
@@ -40,7 +40,7 @@ export type EngagementPreviewLoaderData = {
  * Loads all necessary data for previewing an engagement.
  * Similar to the public engagement loader but uses engagement ID instead of slug.
  */
-export const engagementPreviewLoader = async ({ params }: { params: Params<string> }) => {
+export const engagementPreviewLoader = async ({ params }: LoaderFunctionArgs) => {
     const { engagementId, languageCode } = params;
 
     if (!engagementId) {

web/src/components/engagement/public/view/EngagementLoaderPublic.tsx

@@ -1,4 +1,4 @@
-import { Params } from 'react-router';
+import { LoaderFunctionArgs } from 'react-router';
 import { getAvailableTranslationLanguages, getEngagement, getEngagementBySlug } from 'services/engagementService';
 import { getWidgets } from 'services/widgetService';
 import { getEngagementMetadata, getMetadataTaxa } from 'services/engagementMetadataService';
@@ -36,7 +36,7 @@ export type AwaitedEngagementLoaderPublicData = {
     translationBundle: TranslationBundle;
 };
 
-export const engagementLoaderPublic = async ({ params }: { params: Params<string> }) => {
+export const engagementLoaderPublic = async ({ params }: LoaderFunctionArgs) => {
     const { slug: slugParam, engagementId, language } = params;
     const defaultLanguageCode = AppConfig.language.defaultLanguageId.toLowerCase();
     const activeLanguageCode = (language ?? defaultLanguageCode).toLowerCase();

web/src/components/survey/building/SurveyLoader.tsx

@@ -19,10 +19,10 @@ export type SurveyLoaderData = {
 };
 
 export const SurveyLoader = async ({ params, pattern }: LoaderFunctionArgs) => {
-    const { surveyId, token, engagementId: engagementIdParam, slug: slugParam } = params;
+    const { surveyId: surveyIdParam, token, engagementId: engagementIdParam, slug: slugParam } = params;
+    const surveyId = Number(surveyIdParam);
     const engagementId = Number(engagementIdParam);
-    if (Number.isNaN(Number(surveyId)) && !Number.isNaN(engagementId) && !slugParam)
-        throw new Error('Invalid survey ID');
+    if (Number.isNaN(surveyId) && !Number.isNaN(engagementId) && !slugParam) throw new Error('Invalid survey ID');
     const shouldHaveToken = !pattern.startsWith('/manage'); //non-admin users should have a token
     if (shouldHaveToken && !token) {
         throw new Error('Missing verification token');
@@ -34,18 +34,18 @@ export const SurveyLoader = async ({ params, pattern }: LoaderFunctionArgs) => {
             const engagement = slugParam ? getEngagementBySlug(slugParam) : getEngagement(engagementId);
             const survey = engagement.then((eng) => {
                 if (!eng) throw new Error('Engagement not found for slug: ' + slugParam);
-                if (surveyId && !Number.isNaN(Number(surveyId))) {
-                    return getSurvey(Number(surveyId));
+                if (surveyId && !Number.isNaN(surveyId)) {
+                    return getSurvey(surveyId);
                 }
                 if (eng.surveys && eng.surveys.length > 0) {
                     return eng.surveys[0];
                 }
                 throw new Error('No survey found for engagement with slug: ' + slugParam);
             });
             return { engagement, survey };
-        } else if (surveyId && !Number.isNaN(Number(surveyId))) {
+        } else if (surveyId && !Number.isNaN(surveyId)) {
             // If we are accessing the survey directly via the survey ID
-            const survey = getSurvey(Number(surveyId));
+            const survey = getSurvey(surveyId);
             const engagement = survey.then((surveyData) => {
                 if (!surveyData.engagement_id) {
                     throw new Error('Survey is missing engagement ID');

web/src/components/survey/listing/Surveys.tsx

@@ -454,7 +454,7 @@ const Surveys = () => {
                                     }}
                                 />
                             }
-                            fullWidth={isMediumScreen ? true : false}
+                            fullWidth={isMediumScreen}
                         >
                             Advanced Search
                         </Button>