-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
facebook-post-comment -conversions #4977
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Type: Refactoring
PR Summary: This pull request introduces changes across several files within the Facebook API plugin, focusing on standardizing the naming convention from 'parent_id' to 'parentId' and refining the logic for creating and updating comment conversations. It affects the GraphQL resolvers, message handling logic, model definitions, and the store logic for handling comments. Additionally, it includes UI adjustments in the inbox plugin to conditionally render components based on the conversation's integration kind.
Decision: Comment
📝 Type: 'Refactoring' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that the renaming of 'parent_id' to 'parentId' is consistently applied across the entire codebase to avoid potential mismatches or bugs.
- Consider the implications of the logic changes in the store logic for creating and updating comment conversations, especially in terms of how parent and child comments are handled. It might be beneficial to add more context or documentation around these changes to help future maintainers understand the rationale behind them.
- Review the efficiency of database queries, particularly after creating or updating comment conversations, to minimize the performance impact.
- The PR title and description could be more descriptive to provide better context for the changes made. Including a brief summary of the changes and their impact could help reviewers and future maintainers understand the purpose and scope of the PR more quickly.
packages/plugin-facebook-api/src/store.ts: The proposed code change increases complexity due to added conditional logic, duplication, and debugging statements, making it harder to maintain.
While reviewing the proposed changes, I've noticed an increase in complexity primarily due to additional conditional logic, duplication of logic for creating CommentConversation
and CommentConversationReply
, and the introduction of debugging statements directly in the code. This complexity can make the code harder to maintain and understand.
To improve maintainability and readability, consider the following suggestions:
-
Refactor Common Logic: Both
CommentConversation
andCommentConversationReply
share similar creation logic. Extracting this common logic into a separate function can reduce duplication and make the codebase cleaner. -
Streamline Conditional Logic: The conditions leading to the creation of conversations or replies seem more complex than necessary. Simplifying these conditions can make the code easier to follow.
-
Remove Debugging Statements: Debugging statements like
console.log
are useful during development but should be removed from production code. If logging is necessary, consider using a logging framework that can be enabled or disabled based on the environment. -
Simplify Data Retrieval and Usage: The upfront retrieval of data that's used later in the code can be confusing. It might be beneficial to retrieve and use data closer to where it's actually needed, which can help in understanding the flow of data better.
-
Clarify
conversationId
Handling: The logic aroundconversationId
has become more convoluted. Streamlining howconversationId
is determined and used could make the code more straightforward.
By addressing these points, we can enhance the code's maintainability and readability, making it easier for others to understand and modify in the future.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
@@ -238,10 +240,12 @@ export const getOrCreateComment = async ( | |||
customer: ICustomerDocument, | |||
) => { | |||
const parentCommentConversations = await models.CommentConversation.findOne({ | |||
comment_id: commentParams.comment_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (llm): The addition of a separate query for childCommentConversations
using commentParams.parent_id
as a filter seems redundant given the existing query for parentCommentConversations
with commentParams.comment_id
. This could potentially lead to confusion or bugs if the distinction between parent and child comments is not clear or if their handling diverges in the future.
parentId: commentParams.parent_id, | ||
}); | ||
} | ||
|
||
let conversation; | ||
conversation = await models.CommentConversation.findOne({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): Re-fetching the conversation after potentially creating a new CommentConversation
or CommentConversationReply
seems inefficient. Consider optimizing by using the result of the creation directly if possible, to avoid an additional database query.
|
ISSUE
Context
Your context here. Additionally, any screenshots. Delete this line.
// Delete the below section once completed
PR Checklist