Open
Conversation
Pull Request Test Coverage Report for Build 8973003737Details
💛 - Coveralls |
RichDom2185
requested changes
Apr 15, 2024
Member
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
Hi, thanks for working on this, my comments are as follows (mainly about the feature design):
Comment on lines
+44
to
+49
| export enum StoryStatus { | ||
| Draft = 0, | ||
| Pending, | ||
| Rejected, | ||
| Published | ||
| } |
Member
There was a problem hiding this comment.
Nit, enum values should be UPPER_SNAKE_CASE (e.g. DRAFT)
Comment on lines
+168
to
+291
| {storyLists.published.length > 0 && ( | ||
| <StoriesTable | ||
| title="Published Stories" | ||
| headers={columns} | ||
| stories={storyLists.published | ||
| // Filter out the YAML header from the content | ||
| .map(story => ({ ...story, content: getYamlHeader(story.content).content })) | ||
| .filter( | ||
| story => | ||
| // Always show pinned stories | ||
| story.isPinned || story.authorName.toLowerCase().includes(query.toLowerCase()) | ||
| )} | ||
| storyActions={story => { | ||
| const isAuthor = storiesUserId === story.authorId; | ||
| const hasWritePermissions = | ||
| storiesRole === StoriesRole.Moderator || storiesRole === StoriesRole.Admin; | ||
| return ( | ||
| <StoryActions | ||
| storyId={story.id} | ||
| handleDeleteStory={handleDeleteStory} | ||
| handleTogglePin={handleTogglePinStory} | ||
| handleMovePinUp={handleMovePinUp} | ||
| handleMovePinDown={handleMovePinDown} | ||
| canView // everyone has view permissions, even anonymous users | ||
| canEdit={isAuthor || hasWritePermissions} | ||
| canDelete={isAuthor || hasWritePermissions} | ||
| canPin={hasWritePermissions} | ||
| isPinned={story.isPinned} | ||
| /> | ||
| ); | ||
| }} | ||
| /> | ||
| )} | ||
|
|
||
| {storyLists.pending.length > 0 && ( | ||
| <StoriesTable | ||
| title="Pending Stories" | ||
| headers={columns} | ||
| stories={storyLists.pending | ||
| // Filter out the YAML header from the content | ||
| .map(story => ({ ...story, content: getYamlHeader(story.content).content })) | ||
| .filter( | ||
| story => | ||
| // Always show pinned stories | ||
| story.isPinned || story.authorName.toLowerCase().includes(query.toLowerCase()) | ||
| )} | ||
| storyActions={story => { | ||
| const isAuthor = storiesUserId === story.authorId; | ||
| const hasWritePermissions = | ||
| storiesRole === StoriesRole.Moderator || storiesRole === StoriesRole.Admin; | ||
| return ( | ||
| <StoryActions | ||
| storyId={story.id} | ||
| handleDeleteStory={handleDeleteStory} | ||
| handleRejectStory={handleRejectStory} | ||
| handlePublishStory={handlePublishStory} | ||
| canView // everyone has view permissions, even anonymous users | ||
| canEdit={isAuthor || hasWritePermissions} | ||
| canDelete={isAuthor || hasWritePermissions} | ||
| canModerate={hasWritePermissions} | ||
| isPending={true} | ||
| /> | ||
| ); | ||
| }} | ||
| /> | ||
| )} | ||
|
|
||
| {storyLists.rejected.length > 0 && ( | ||
| <StoriesTable | ||
| title="Rejected Stories" | ||
| headers={columns} | ||
| stories={storyLists.rejected | ||
| // Filter out the YAML header from the content | ||
| .map(story => ({ ...story, content: getYamlHeader(story.content).content })) | ||
| .filter( | ||
| story => | ||
| // Always show pinned stories | ||
| story.isPinned || story.authorName.toLowerCase().includes(query.toLowerCase()) | ||
| )} | ||
| storyActions={story => { | ||
| const isAuthor = storiesUserId === story.authorId; | ||
| const hasWritePermissions = | ||
| storiesRole === StoriesRole.Moderator || storiesRole === StoriesRole.Admin; | ||
| return ( | ||
| <StoryActions | ||
| storyId={story.id} | ||
| handleDeleteStory={handleDeleteStory} | ||
| canView // everyone has view permissions, even anonymous users | ||
| canEdit={isAuthor || hasWritePermissions} | ||
| canDelete={isAuthor || hasWritePermissions} | ||
| /> | ||
| ); | ||
| }} | ||
| /> | ||
| )} | ||
|
|
||
| {storyLists.draft.length > 0 && ( | ||
| <StoriesTable | ||
| title="Draft Stories" | ||
| headers={columns} | ||
| stories={storyLists.draft | ||
| // Filter out the YAML header from the content | ||
| .map(story => ({ ...story, content: getYamlHeader(story.content).content })) | ||
| .filter( | ||
| story => | ||
| // Always show pinned stories | ||
| story.isPinned || story.authorName.toLowerCase().includes(query.toLowerCase()) | ||
| )} | ||
| storyActions={story => { | ||
| const isAuthor = storiesUserId === story.authorId; | ||
| const hasWritePermissions = | ||
| storiesRole === StoriesRole.Moderator || storiesRole === StoriesRole.Admin; | ||
| return ( | ||
| <StoryActions | ||
| storyId={story.id} | ||
| handleDeleteStory={handleDeleteStory} | ||
| canView // everyone has view permissions, even anonymous users | ||
| canEdit={isAuthor || hasWritePermissions} | ||
| canDelete={isAuthor || hasWritePermissions} | ||
| /> | ||
| ); | ||
| }} | ||
| /> | ||
| )} |
Member
There was a problem hiding this comment.
There's extensive code duplication here:
Comment on lines
+83
to
+91
| status === StoryStatus.Draft | ||
| ? '/draft' | ||
| : status === StoryStatus.Pending | ||
| ? '/pending' | ||
| : status === StoryStatus.Rejected | ||
| ? '/rejected' | ||
| : status === StoryStatus.Published | ||
| ? '/published' | ||
| : ''; |
Member
There was a problem hiding this comment.
Filters are generally done using a query parameter instead of a different route/path (which would conventionally mean a different resource.)
Comment on lines
+38
to
+64
|
|
||
| const draftStories: StoryListView[] = yield call(async () => { | ||
| const resp = await getStories(tokens, StoryStatus.Draft); | ||
| return resp ?? []; | ||
| }); | ||
|
|
||
| const pendingStories: StoryListView[] = yield call(async () => { | ||
| const resp = await getStories(tokens, StoryStatus.Pending); | ||
| return resp ?? []; | ||
| }); | ||
|
|
||
| const rejectedStories: StoryListView[] = yield call(async () => { | ||
| const resp = await getStories(tokens, StoryStatus.Rejected); | ||
| return resp ?? []; | ||
| }); | ||
|
|
||
| const publishedStories: StoryListView[] = yield call(async () => { | ||
| const resp = await getStories(tokens, StoryStatus.Published); | ||
| return resp ?? []; | ||
| }); | ||
|
|
||
| const allStories: StoryListViews = { | ||
| draft: draftStories, | ||
| pending: pendingStories, | ||
| rejected: rejectedStories, | ||
| published: publishedStories | ||
| }; |
Member
There was a problem hiding this comment.
This design doesn't look right to me. To the user/the frontend, the "listing" of stories should be transparent.
In other words, the FE just makes one "list stories" query, and the backend returns a list of stories that the user has access to. The permissions/different types of stories should be transparent and the frontend can subsequently later filter the story by the status. You are adding a status field inside the story model in the backend but it's not being utilised here at all.
…nto stories-moderation
…nto stories-moderation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
source-academy/stories-backend#120
Type of change
How to test
Checklist