Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IDOL, shoutouts] - edit shoutouts #578

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
12 changes: 12 additions & 0 deletions backend/src/API/shoutoutAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ export const hideShoutout = async (
await shoutoutsDao.updateShoutout({ ...shoutout, hidden: hide });
};

export const editShoutout = async (
uuid: string,
newMessage: string,
user: IdolMember
): Promise<Shoutout> => {
const shoutout = await shoutoutsDao.getShoutout(uuid);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add permissions checking here to make sure that the shoutout belongs to the user who's trying to edit it OR it's a leadOrAdmin?

if (!shoutout) {
throw new NotFoundError(`Shoutout with uuid: ${uuid} does not exist!`);
}
return shoutoutsDao.editShoutout(uuid, newMessage);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see if you can use updateShoutout here instead

};

/**
* Deletes a shoutout, ensuring the user has the necessary permissions or ownership.
* @throws {NotFoundError} If no shoutout with the provided uuid is found.
Expand Down
10 changes: 8 additions & 2 deletions backend/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import * as winston from 'winston';
import * as expressWinston from 'express-winston';
import { app as adminApp, env } from './firebase';
import PermissionsManager from './utils/permissionsManager';
import { HandlerError } from './utils/errors';
import {
acceptIDOLChanges,
getIDOLChangesPR,
Expand All @@ -31,7 +30,8 @@ import {
getShoutouts,
giveShoutout,
hideShoutout,
deleteShoutout
deleteShoutout,
editShoutout
} from './API/shoutoutAPI';
import {
allSignInForms,
Expand Down Expand Up @@ -86,6 +86,7 @@ import {
import DPSubmissionRequestLogDao from './dao/DPSubmissionRequestLogDao';
import AdminsDao from './dao/AdminsDao';
import { sendMail } from './API/mailAPI';
import { HandlerError } from './utils/errors';

// Constants and configurations
const app = express();
Expand Down Expand Up @@ -275,6 +276,11 @@ loginCheckedPut('/shoutout', async (req, user) => {
return {};
});

loginCheckedPut('/shoutout/:uuid', async (req, user) => {
await editShoutout(req.params.uuid, req.body.message, user);
return {};
});

loginCheckedDelete('/shoutout/:uuid', async (req, user) => {
await deleteShoutout(req.params.uuid, user);
return {};
Expand Down
14 changes: 14 additions & 0 deletions backend/src/dao/ShoutoutsDao.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,20 @@ export default class ShoutoutsDao extends BaseDao<Shoutout, DBShoutout> {
return this.createDocument(shoutoutWithUUID.uuid, shoutoutWithUUID);
}

/**
* Edits a shoutout
* @param uuid - uuid of the shoutout
* @param shoutout - shoutout object
*/
async editShoutout(uuid: string, newMessage: string): Promise<Shoutout> {
await this.collection.doc(uuid).update({ message: newMessage });
const updatedDoc = await this.getDocument(uuid);
if (!updatedDoc) {
throw new Error('Failed to fetch updated shoutout...');
Copy link
Collaborator

@andrew032011 andrew032011 Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the ... in the string?

EDIT: also see Patricia's comment, we shouldn't be doing permissions checks here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could you use updateShoutout instead of creating a method just to edit the message?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more for future reference, but I think checking if the document exists in the DAO layer might be repetitive, since we already check if the uuid exists in the API layer. Not including the checks here also helps keep the DAO files consistent, since most of them do not have additional checks.

Copy link
Collaborator

@andrew032011 andrew032011 Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah thanks for catching this @patriciaahuang, not sure how I didn't see it. We should not do any permissions checks at the DAO layer. That's reserved for the API layer! but since we're likely deleting this function anyways, yeah it's good to know for future reference.

}
return updatedDoc;
}

/**
* Updates a shoutout
* @param shoutout - shoutout object
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/API/ShoutoutsAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ export default class ShoutoutsAPI {
return APIWrapper.put(`${backendURL}/shoutout`, { uuid, hide }).then((res) => res.data);
}

public static updateShoutout(
uuid: string,
shoutout: Partial<Shoutout>
): Promise<ShoutoutResponseObj> {
return APIWrapper.put(`${backendURL}/shoutout/${uuid}`, shoutout).then((res) => res.data);
}

public static async deleteShoutout(uuid: string): Promise<void> {
await APIWrapper.delete(`${backendURL}/shoutout/${uuid}`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,22 @@
display: inline-block;
padding-left: 1.5rem;
padding-bottom: 1rem;
align-self: left;
}

.shoutoutDelete {
display: flex;
justify-content: space-between;
align-items: center;
margin-top: 0 !important;
margin-bottom: 0 !important;
}

.shoutoutActions {
display: flex;
justify-content: space-between;
align-items: center;
padding: 0 1.5rem;
margin-top: 1rem;
margin-right: 1rem;
margin-bottom: 1rem;
}
78 changes: 64 additions & 14 deletions frontend/src/components/Forms/ShoutoutsPage/ShoutoutCard.tsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,80 @@
import { Card } from 'semantic-ui-react';
import React, { useState, Dispatch, SetStateAction } from 'react';
import { Button, Card, Form, Icon, Modal, TextArea } from 'semantic-ui-react';
import ShoutoutsAPI from '../../../API/ShoutoutsAPI';
import ShoutoutDeleteModal from '../../Modals/ShoutoutDeleteModal';
import styles from './ShoutoutCard.module.css';

const ShoutoutCard = (props: {
interface ShoutoutCardProps {
shoutout: Shoutout;
setGivenShoutouts: React.Dispatch<React.SetStateAction<Shoutout[]>>;
}): JSX.Element => {
const { shoutout, setGivenShoutouts } = props;
setGivenShoutouts: Dispatch<SetStateAction<Shoutout[]>>;
}

const fromString = shoutout.isAnon
? 'From: Anonymous'
: `From: ${shoutout.giver?.firstName} ${shoutout.giver?.lastName}`;
const dateString = `${new Date(shoutout.timestamp).toDateString()}`;
const ShoutoutCard: React.FC<ShoutoutCardProps> = ({ shoutout, setGivenShoutouts }) => {
const [isEditing, setIsEditing] = useState(false);
const [editedMessage, setEditedMessage] = useState(shoutout.message);

const handleEditShoutout = async () => {
try {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do without this try/catch and console statement, since it doesn't do much other than console.log the error (which we would get in the console anyways). It would be worth it to try/catch if we had some sort of fallback code we wanted to run or wanted to record metrics.

await ShoutoutsAPI.updateShoutout(shoutout.uuid, { message: editedMessage });
setGivenShoutouts((prevShoutouts) =>
prevShoutouts.map((s) => (s.uuid === shoutout.uuid ? { ...s, message: editedMessage } : s))
);
setIsEditing(false);
} catch (error) {
console.error('Failed to edit shoutout:', error);

Check warning on line 24 in frontend/src/components/Forms/ShoutoutsPage/ShoutoutCard.tsx

View workflow job for this annotation

GitHub Actions / check

Unexpected console statement
}
};

return (
<Card className={styles.shoutoutCardContainer}>
<Card.Group widths="equal" className={styles.shoutoutCardDetails}>
<Card.Content header={`To: ${shoutout.receiver}`} className={styles.shoutoutTo} />
<Card.Content className={styles.shoutoutDate} content={dateString} />
</Card.Group>
<Card.Group widths="equal" className={styles.shoutoutDelete}>
<Card.Meta className={styles.shoutoutFrom} content={fromString} />
<ShoutoutDeleteModal uuid={shoutout.uuid} setGivenShoutouts={setGivenShoutouts} />
<Card.Content
className={styles.shoutoutDate}
content={new Date(shoutout.timestamp).toDateString()}
/>
</Card.Group>
<div className={styles.shoutoutActions}>
<Card.Meta
className={styles.shoutoutFrom}
content={
shoutout.isAnon
? 'From: Anonymous'
: `From: ${shoutout.giver.firstName} ${shoutout.giver.lastName}`
}
/>
<div>
<ShoutoutDeleteModal uuid={shoutout.uuid} setGivenShoutouts={setGivenShoutouts} />
<Button icon onClick={() => setIsEditing(true)}>
<Icon name="edit" />
</Button>
</div>
</div>
<Card.Content description={shoutout.message} />
<Modal open={isEditing} onClose={() => setIsEditing(false)}>
<Modal.Header>Edit Shoutout</Modal.Header>
<Modal.Content>
<Form>
<Form.Field>
<label>Message</label>
<TextArea
value={editedMessage}
onChange={(e, { value }) => setEditedMessage((value || '') as string)}
/>
</Form.Field>
</Form>
</Modal.Content>
<Modal.Actions>
<Button onClick={() => setIsEditing(false)} negative>
Cancel
</Button>
<Button onClick={handleEditShoutout} positive>
Save
</Button>
</Modal.Actions>
</Modal>
</Card>
);
};

export default ShoutoutCard;
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.trashIcon {
position: absolute;
right: 1%;
margin-top: 2rem;
position: relative;
right: 10%;
}
Loading