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

edit and delete your user listing #65

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

Conversation

parth-agrawal
Copy link
Contributor

@parth-agrawal parth-agrawal commented Aug 17, 2024

  • delete functionality works and can see all buttons but edit does not work
  • able to edit userlistings
  • delete functionality works
  • delete cruft

🚀 This description was created by Ellipsis for commit e0d6712

Summary:

This PR adds edit and delete functionalities for user listings, updating backend and frontend components.

Key points:

  • Adds edit and delete functionalities for user listings.
  • Updates backend services and frontend components.
  • controller.ts: Added router.put for updating listings by ID.
  • interface.ts: Updated updateUserListing to accept new parameters.
  • service.ts: Modified updateUserListing for updates.
  • ProfileBanner.tsx: Added edit and delete buttons.
  • DeleteListingModal.tsx: Implemented async delete.
  • UserListingModal.tsx: Added logic for creation and update.
  • PeopleListingSection.tsx: Refreshed listings on changes.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to d9da6c9 in 24 seconds

More details
  • Looked at 286 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/components/compound/Modal/UserListingModal.tsx:74
  • Draft comment:
    The logic in UserListingModal assumes a user can only have one listing by using listingExists derived from the length of userListings. If the application allows multiple listings per user, this could lead to incorrect behavior. Consider revising this logic to handle multiple listings appropriately.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment addresses a potential flaw in the logic of the code, which is relevant to the changes made in the PR. It suggests a specific area where the logic might fail if the application requirements change to allow multiple listings per user. This is a valid concern and could lead to incorrect behavior if not addressed.
    The comment is speculative as it assumes a future scenario where multiple listings are allowed, which may not be the current requirement. The current implementation might be correct for the existing requirements.
    Even though the comment is speculative, it highlights a potential issue that could arise if the application's requirements change. It's better to consider such possibilities during development to avoid future issues.
    The comment should be kept as it highlights a potential issue with the current logic that could lead to incorrect behavior if the application's requirements change to allow multiple listings per user.

Workflow ID: wflow_3yjT2pCYNMRhXdRW


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant