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

Database redesign #136

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Database redesign #136

wants to merge 13 commits into from

Conversation

georgimld
Copy link
Contributor

@georgimld georgimld commented Nov 27, 2024

Changes in this PR:

  • updated data model to combine all Comment tables (Comment, NewsComment, PostComment, ProjectComment, CollaborationComment) into a single table Comment
  • migrate data from old tables into new one (in the migration file)
  • update all types, requests and queries
  • add a separate CommentLike table (like for comments)
  • update the old Like comment (rename projectId -> objectId, add objectType) -> for now we only have project likes but we could extend it

How to test:

  • build all containers in main (build db from scratch e.g. docker compose up --build)
  • make sure your strapi has data
  • add some data to the database (either manually or via a postgres dump)
    • psql "postgresql://inno:hub@localhost:5432/innohub" < ...path_to_dump.sql
    • connect to psql via psql "postgresql://inno:hub@localhost:5432/innohub"
    • look & check the old table format
  • do npm run prisma migrate deploy in the new branch (this migration should be applied 20241217173509_comment_model)
  • the result should be something like this:
image
  • make sure you update the cache by removing the "sync" entry from the redis UI, and then triggering the /api/redis/full-refresh endpoint

other useful psql commands:

  • to drop everything from the DB drop schema innohub CASCADE;
  • to set the "innohub" schema as the current search path in psql -> SET search_path TO innohub;

Closes #135

@georgimld georgimld self-assigned this Nov 27, 2024
@georgimld georgimld marked this pull request as ready for review January 27, 2025 12:21
@andrea-smiesna andrea-smiesna self-requested a review January 27, 2025 12:25
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try to avoid type definitions that are independent of our Prisma Schema where possible. This would avoid having to use casts like in L49 of CollaborationQuestionCard.tsx, where we know the type is correct because the the database gives us a createdAt property for every comment but it doesn't align with the type we defined elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to issue #158

@@ -41,9 +46,9 @@ export const CollaborationQuestionCard = ({
}: CollaborationQuestionCardProps) => {
const { title, description, authors, comments: projectComments } = content;
const { setCollaborationCommentsAmount } = useProject();
const sortedProjectComments = sortDateByCreatedAtDesc(projectComments);
const sortedProjectComments = sortDateByCreatedAtDesc(projectComments as CollaborationCommentWithDate[]); //todo check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the todo if it's done :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

return (
<Stack direction={'row'}>
<UpvoteControls upvoteCount={upvoteCount} isUpvoted={isUpvoted} onUpvote={onUpvote} sx={{ mr: 0.5 }} />
{/* <UpvoteControls upvoteCount={upvoteCount} isLiked={isLiked} onUpvote={onUpvote} sx={{ mr: 0.5 }} /> */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the "UpvoteControls" component used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced with LikeControls and removed UpvoteControls ✅

upvoteCount,
isUpvoted,
onUpvote,
// likeCount,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed if it's not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed ✅

@@ -34,6 +34,7 @@
"components_collaboration_comments_collaborationCommentCard_upvoteError": "Der Kommentar konnte nicht hochgestuft werden. Bitte versuche es später noch einmal.",
"components_collaboration_comments_collaborationCommentCard_updateError": "Der Kommentar konnte nicht aktualisiert werden. Bitte versuche es später noch einmal.",
"components_collaboration_comments_collaborationCommentCard_deleteError": "Der Kommentar konnte nicht gelöscht werden. Bitte versuche es später noch einmal.",
"components_collaboration_comments_collaborationCommentCard_questionMissing": "Die Frage konnte nicht gefunden werden. Bitte versuche es später noch einmal.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The translation is not used anywhere within the collaboration comment card

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed ✅

commentId: string;
comment: string;
author?: string;
// commentId: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed ✅

return comments.map((comment: RedisHashedNewsComment) => {
//todo add likes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remaining todo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed ✅

@@ -73,11 +73,6 @@
"relation": "manyToMany",
"target": "api::opportunity.opportunity",
"mappedBy": "participants"
},
"username": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The username field should be kept in Strapi "InnoUser" collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed forgotten files ✅

@@ -928,7 +928,6 @@ export interface ApiInnoUserInnoUser extends Schema.CollectionType {
'manyToMany',
'api::opportunity.opportunity'
>;
username: Attribute.String & Attribute.Unique;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to the "username" field, should be updated automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed forgotten files ✅

return comment?.likes;
}

//todo check this ObjectType.COMMENT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this todo still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, removed (refactored too)

const getUpvotes = comments.map(async (comment): Promise<Comment> => {
const { data: isUpvotedByUser } = await isProjectCommentUpvotedByUser({ commentId: comment.id });
const dbComments = await getCommentsByObjectId(dbClient, body.projectId);
const comments = await Promise.all(dbComments.map((comment) => mapToComment(comment)));
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 that we want the comments sorted by createdAt DESC (sortDateByCreatedAtDesc method) or sort them in the Comments Card.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not done, sorry :(

const commentCount = await getCommentResponseCount(dbClient, deletedResponse.parentId);
await updateCollaborationCommentInCache({ user, comment: { id: deletedResponse.parentId, commentCount } });
} else {
logger.info('no parentID');
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 that the logging can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const body = { id: comment.objectId, commentCount };
return comment.objectType === 'POST'
? await updatePostInCache({ post: body, user: author })
: await updateProjectUpdateInCache({ update: body });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why we're saving author of the POST, but not of the UPDATE to the cache?

const author = await getInnoUserByProviderId(comment.author);
const likedBy = await Promise.allSettled(
comment.likes.map(async (like) => await getInnoUserByProviderId(like.likedBy)),
).then((results) => getFulfilledResults(results));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a helper function which makes the "promises" a bit less complicated syntax wise. Could you please change this to:
`const getLikedBy = comment.likes.map(async (like) => await getInnoUserByProviderId(like.likedBy));

const likedBy = await getPromiseResults(getLikedBy);`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

question: CollaborationQuestion,
): Promise<CollaborationComment[]> => {
return await Promise.allSettled(
comments.map(async (comment) => await mapToCollborationComment(comment, question)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here as on line 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done too ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database redesign
3 participants