Skip to content

Commit

Permalink
Merge pull request #2390 from HHS/main
Browse files Browse the repository at this point in the history
[Prod] Fixes from accessibility review
  • Loading branch information
Jones-QuarteyDana authored Sep 30, 2024
2 parents cb25068 + 742ed43 commit 0037157
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 14 deletions.
17 changes: 12 additions & 5 deletions frontend/src/components/GoalForm/ResourceRepeater.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import { isValidResourceUrl } from '@ttahub/common';
import PropTypes from 'prop-types';
import { v4 as uuidv4 } from 'uuid';
import {
FormGroup, Label, Button, Fieldset,
FormGroup,
Label,
Button,
Fieldset,
ErrorMessage,
} from '@trussworks/react-uswds';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faTrash } from '@fortawesome/free-solid-svg-icons';
Expand All @@ -22,7 +26,7 @@ export default function ResourceRepeater({
isLoading,
}) {
const addResource = () => {
if ((error.props.children) || resources.some((r) => !r.value)) {
if ((error) || resources.some((r) => !r.value)) {
return;
}
const newResources = [...resources, { key: uuidv4(), value: '' }];
Expand All @@ -45,7 +49,7 @@ export default function ResourceRepeater({

return (
<>
<FormGroup error={error.props.children}>
<FormGroup error={!!(error)}>
<div>
<Fieldset>
<legend>
Expand All @@ -58,7 +62,9 @@ export default function ResourceRepeater({
Enter one resource per field. To enter more resources, select “Add new resource”
</span>
</Fieldset>
{error.props.children ? error : null}
<ErrorMessage>
{error}
</ErrorMessage>
<div className="ttahub-resource-repeater">
{ resources.map((r, i) => (
<div key={r.key} className={`display-flex${r.value && !isValidResourceUrl(r.value) ? ' ttahub-resource__error' : ''}`} id="resources">
Expand Down Expand Up @@ -103,13 +109,14 @@ ResourceRepeater.propTypes = {
value: PropTypes.string,
})).isRequired,
setResources: PropTypes.func.isRequired,
error: PropTypes.node.isRequired,
error: PropTypes.string,
validateResources: PropTypes.func.isRequired,
isLoading: PropTypes.bool,
toolTipText: PropTypes.string,
};

ResourceRepeater.defaultProps = {
error: '',
isLoading: false,
toolTipText: 'Copy & paste web address of TTA resource used for this objective. Usually an ECLKC page.',
};
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,7 @@ export default function Objective({
resources={resourcesForRepeater}
isOnReport={isOnReport || false}
setResources={onChangeResources}
error={errors.resources
? ERROR_FORMAT(errors.resources.message)
: NO_ERROR}
error={errors.resources ? errors.resources.message : ''}
validateResources={onBlurResources}
inputName={objectiveResourcesInputName}
userCanEdit
Expand Down
5 changes: 2 additions & 3 deletions frontend/src/pages/ActivityReport/__tests__/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1173,9 +1173,8 @@ describe('ActivityReport', () => {
userEvent.click(saveGoal);
});

const errors = document.querySelectorAll('.usa-error-message');
expect(errors.length).toBe(1);
// they'll be one at least (objective suspend reason modal error sits in the dom at all times)
const errors = document.querySelectorAll('.usa-error-message:not(:empty)');
expect(errors.length).toBe(0);

await waitFor(() => {
expect(fetchMock.called('/api/activity-reports/1', { method: 'PUT' })).toBe(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ const EventSummary = ({
</FormItem>
</div>
<div className="margin-top-2" data-testid="creator-select">
<Label htmlFor="creatorName">
<Label htmlFor="ownerId">
Event creator
<Req />
</Label>
Expand Down
2 changes: 2 additions & 0 deletions src/models/goal.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
afterCreate,
afterUpdate,
afterDestroy,
beforeCreate,
} = require('./hooks/goal');
const { GOAL_CREATED_VIA, CREATION_METHOD } = require('../constants');

Expand Down Expand Up @@ -152,6 +153,7 @@ export default (sequelize, DataTypes) => {
hooks: {
beforeValidate: async (instance, options) => beforeValidate(sequelize, instance, options),
beforeUpdate: async (instance, options) => beforeUpdate(sequelize, instance, options),
beforeCreate: async (instance, options) => beforeCreate(sequelize, instance, options),
afterCreate: async (instance, options) => afterCreate(sequelize, instance, options),
afterUpdate: async (instance, options) => afterUpdate(sequelize, instance, options),
afterDestroy: async (instance, options) => afterDestroy(sequelize, instance, options),
Expand Down
33 changes: 31 additions & 2 deletions src/models/hooks/goal.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
const { Op } = require('sequelize');
const { GOAL_STATUS, GOAL_COLLABORATORS, OBJECTIVE_STATUS } = require('../../constants');
const {
GOAL_STATUS,
GOAL_COLLABORATORS,
OBJECTIVE_STATUS,
CREATION_METHOD,
} = require('../../constants');
const {
currentUserPopulateCollaboratorForType,
} = require('../helpers/genericCollaborator');
Expand Down Expand Up @@ -28,7 +33,25 @@ const findOrCreateGoalTemplate = async (sequelize, transaction, regionId, name,
},
transaction,
});
return { id: goalTemplate[0].id, name };
return { id: goalTemplate[0].id, name, creationMethod: goalTemplate[0].creationMethod };
};

const checkForCuratedGoal = async (sequelize, instance) => {
// we don't want to be setting goalTemplateId if it's already set
if (instance.goalTemplateId) return;

const curatedTemplate = await sequelize.models.GoalTemplate.findOne({
where: {
hash: sequelize.fn('md5', sequelize.fn('NULLIF', sequelize.fn('TRIM', instance.name), '')),
creationMethod: CREATION_METHOD.CURATED,
regionId: null,
},
attributes: ['id'],
});

if (curatedTemplate) {
instance.set('goalTemplateId', curatedTemplate.id);
}
};

const autoPopulateOnAR = (_sequelize, instance, options) => {
Expand Down Expand Up @@ -299,6 +322,10 @@ const beforeValidate = async (sequelize, instance, options) => {
preventNameChangeWhenOnApprovedAR(sequelize, instance, options);
};

const beforeCreate = async (sequelize, instance, options) => {
await checkForCuratedGoal(sequelize, instance, options);
};

const beforeUpdate = async (sequelize, instance, options) => {
preventNameChangeWhenOnApprovedAR(sequelize, instance, options);
await preventCloseIfObjectivesOpen(sequelize, instance, options);
Expand Down Expand Up @@ -329,10 +356,12 @@ export {
autoPopulateOnApprovedAR,
preventNameChangeWhenOnApprovedAR,
preventCloseIfObjectivesOpen,
checkForCuratedGoal,
propagateName,
beforeValidate,
beforeUpdate,
afterCreate,
afterUpdate,
beforeCreate,
afterDestroy,
};
49 changes: 49 additions & 0 deletions src/models/hooks/goal.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const {
processForEmbeddedResources,
findOrCreateGoalTemplate,
preventCloseIfObjectivesOpen,
beforeCreate,
} = require('./goal');
const { GOAL_STATUS, OBJECTIVE_STATUS } = require('../../constants');
const { createRecipient, createGrant, createGoal } = require('../../testUtils');
Expand All @@ -18,6 +19,54 @@ const {
jest.mock('../../services/resource');

describe('goal hooks', () => {
describe('beforeCreate', () => {
it('does nothing if instance already has goalTemplateId', async () => {
const instanceSet = jest.fn();
const instance = {
goalTemplateId: 1,
set: instanceSet,
};
await expect(beforeCreate({}, instance)).resolves.not.toThrow();
expect(instanceSet).not.toHaveBeenCalled();
});

it('does nothing if sequelize cannot find a curated template', async () => {
const instanceSet = jest.fn();
const instance = {
goalTemplateId: null,
set: instanceSet,
};
const sequelize = {
fn: jest.fn(),
models: {
GoalTemplate: {
findOne: jest.fn().mockResolvedValue(null),
},
},
};
await expect(beforeCreate(sequelize, instance)).resolves.not.toThrow();
expect(instanceSet).not.toHaveBeenCalled();
});

it('sets goalTemplateId if a curated template is found', async () => {
const instanceSet = jest.fn();
const instance = {
goalTemplateId: null,
set: instanceSet,
};
const sequelize = {
fn: jest.fn(),
models: {
GoalTemplate: {
findOne: jest.fn().mockResolvedValue({ id: 1 }),
},
},
};
await expect(beforeCreate(sequelize, instance)).resolves.not.toThrow();
expect(instanceSet).toHaveBeenCalledWith('goalTemplateId', 1);
});
});

describe('preventCloseIfObjectivesOpen', () => {
it('does nothing if instance.changed is not an array', async () => {
const instance = {
Expand Down

0 comments on commit 0037157

Please sign in to comment.