[IDOL, shoutouts] - edit shoutouts #578
Conversation
|
[diff-counting] Significant lines: 130. |
kevinmram
assigned kevinmram and andrew032011 and unassigned kevinmram and andrew032011
Can you make it so that the spacing around the icon doesn't change as a result of adding this icon? The layout looks a little funky now since the icon is larger than the trash icon.
|
| await this.collection.doc(uuid).update({ message: newMessage }); | ||
| const updatedDoc = await this.getDocument(uuid); | ||
| if (!updatedDoc) { | ||
| throw new Error('Failed to fetch updated shoutout...'); |
There was a problem hiding this comment.
Remove the ... in the string?
EDIT: also see Patricia's comment, we shouldn't be doing permissions checks here
There was a problem hiding this comment.
Also, could you use updateShoutout instead of creating a method just to edit the message?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if (!shoutout) { | ||
| throw new NotFoundError(`Shoutout with uuid: ${uuid} does not exist!`); | ||
| } | ||
| return shoutoutsDao.editShoutout(uuid, newMessage); |
There was a problem hiding this comment.
see if you can use updateShoutout here instead
| const [editedMessage, setEditedMessage] = useState(shoutout.message); | ||
|
|
||
| const handleEditShoutout = async () => { | ||
| try { |
There was a problem hiding this comment.
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.
| newMessage: string, | ||
| user: IdolMember | ||
| ): Promise<Shoutout> => { | ||
| const shoutout = await shoutoutsDao.getShoutout(uuid); |
There was a problem hiding this comment.
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?
| await this.collection.doc(uuid).update({ message: newMessage }); | ||
| const updatedDoc = await this.getDocument(uuid); | ||
| if (!updatedDoc) { | ||
| throw new Error('Failed to fetch updated shoutout...'); |
There was a problem hiding this comment.
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.
added a button and functions for editing shoutouts , allowing users to edit their shoutouts after submission.
editing-shoutout.s.mp4