feat: Implement pause listing functionality #284
Draft
Conversation
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
- Add pause application service following cancel pattern - Add pauseItemListing GraphQL mutation and resolver - Update UI components with pause button and confirmation modal - Filter paused listings from reservers' search results - Add state mapping for domain to UI status conversion Implements pause listing feature per issue #42 requirements. Allows sharers to pause active listings, removing them from search results while retaining reservation history.
Contributor
Reviewer's GuideThis PR adds a ‘pause listing’ feature by introducing a new application service with transaction scoping, extending the GraphQL API with a pauseItemListing mutation and filtering logic, and integrating the pause action into the client UI with state mapping and confirmation dialogs. Sequence diagram for pausing a listing via GraphQL mutationsequenceDiagram
actor User
participant UI
participant GraphQL
participant ApplicationService
participant DataSource
User->>UI: Clicks "Pause" button
UI->>GraphQL: pauseItemListing(id)
GraphQL->>ApplicationService: pause({ id })
ApplicationService->>DataSource: withScopedTransaction(repo)
DataSource->>ApplicationService: getById(id)
ApplicationService->>DataSource: listing.pause() & repo.save(listing)
DataSource-->>ApplicationService: Paused listing entity
ApplicationService-->>GraphQL: Paused listing entity
GraphQL-->>UI: Paused listing entity
UI-->>User: Show success message
Class diagram for ItemListingApplicationService and pause commandclassDiagram
class ItemListingApplicationService {
+cancel(command)
+pause(command)
+queryPaged(command)
+update(command)
// ... other methods
}
class ItemListingPauseCommand {
+id: string
}
ItemListingApplicationService --> ItemListingPauseCommand
class ItemListing {
+pause()
+state: string
// ... other fields and methods
}
ItemListingApplicationService --> ItemListing
ItemListingPauseCommand --> ItemListing
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Add an ownership check in pauseItemListing (resolver or service) to ensure only the sharer who owns the listing can pause it, not just any authenticated user.
- Instead of filtering out paused listings in memory in the itemListings resolver, push that filter into your data source query or service to avoid loading all listings and improve performance.
- The Popconfirm + Button logic is duplicated in both card and table components—consider extracting a reusable action component to reduce duplication and keep the UI code DRY.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add an ownership check in pauseItemListing (resolver or service) to ensure only the sharer who owns the listing can pause it, not just any authenticated user.
- Instead of filtering out paused listings in memory in the itemListings resolver, push that filter into your data source query or service to avoid loading all listings and improve performance.
- The Popconfirm + Button logic is duplicated in both card and table components—consider extracting a reusable action component to reduce duplication and keep the UI code DRY.
## Individual Comments
### Comment 1
<location> `packages/sthrift/graphql/src/schema/types/listing/item-listing.resolvers.ts:173-177` </location>
<code_context>
const result =
await context.applicationServices.Listing.ItemListing.pause({
id: args.id,
});
return result;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return await context.applicationServices.Listing.ItemListing.pause({
id: args.id,
});
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
packages/sthrift/graphql/src/schema/types/listing/item-listing.resolvers.ts
Outdated
Show resolved
Hide resolved
- Add GraphQL resolver tests for pauseItemListing mutation - Add application service unit tests for pause functionality - Add container component tests with state mapping verification - Update Storybook stories with pause action scenarios - Add feature file scenarios for pause listing resolver Completes test coverage requirements per instruction markdowns.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements pause listing functionality for sharers, allowing them to temporarily remove active listings from search results while preserving reservation history. The implementation follows the established cancel pattern and adheres to the DDD architecture with proper separation between domain, application, and presentation layers.
Key Changes:
- Added pause application service following the Unit of Work pattern with transactional scoping
- Implemented
pauseItemListingGraphQL mutation with authentication checks and comprehensive test coverage - Enhanced UI with Popconfirm modal for pause confirmation and state mapping from domain states to UI statuses
- Applied filtering logic to exclude paused listings from reserver search results
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sthrift/application-services/src/contexts/listing/item/pause.ts | New application service implementing pause functionality with UoW pattern, matching cancel service structure |
| packages/sthrift/application-services/src/contexts/listing/item/pause.test.ts | Comprehensive unit tests covering success case, not-found error, save failure, and repository errors |
| packages/sthrift/application-services/src/contexts/listing/item/index.ts | Registered pause service in ItemListing application service interface and implementation |
| packages/sthrift/graphql/src/schema/types/listing/item-listing.graphql | Added pauseItemListing mutation to GraphQL schema following cancelItemListing pattern |
| packages/sthrift/graphql/src/schema/types/listing/item-listing.resolvers.ts | Implemented pauseItemListing mutation resolver with authentication and filtering logic for itemListings query |
| packages/sthrift/graphql/src/schema/types/listing/item-listing.resolvers.test.ts | Added BDD-style test scenarios for pause mutation covering authentication, errors, and edge cases |
| packages/sthrift/graphql/src/schema/types/listing/features/item-listing.resolvers.feature | Added feature scenarios documenting pause mutation behavior for successful operations, authentication failures, and errors |
| apps/ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-table.tsx | Replaced plain button with Popconfirm component for pause action with user-friendly confirmation message |
| apps/ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-card.tsx | Added Popconfirm to pause button in card view matching table implementation |
| apps/ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-table.container.tsx | Implemented pauseListing mutation hook, added domain-to-UI state mapping function, and integrated pause action handling |
| apps/ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-table.container.test.tsx | Added tests verifying component rendering and state mapping for Published and Paused listings |
| apps/ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-table.container.graphql | Added pauseItemListing mutation document requesting id, state, and updatedAt fields |
| apps/ui-sharethrift/src/components/layouts/home/my-listings/stories/all-listings-table.stories.tsx | Added Storybook stories demonstrating pause action, active listings filtering, and paused listings display |
Comment on lines
49
to
51
| // Filter out paused listings from search results for reservers | ||
| // Paused listings should not be visible to reservers | ||
| return allListings.filter(listing => listing.state !== 'Paused'); |
There was a problem hiding this comment.
The itemListings query filters only 'Paused' listings, but should also filter out other non-active states like 'Cancelled', 'Drafted', 'Expired', 'Blocked', and 'Appeal Requested'. According to the domain value objects, only 'Published' listings should be visible to reservers in search results.
Consider filtering to only show 'Published' listings:
return allListings.filter(listing => listing.state === 'Published');This aligns better with the isActive property in the ListingState value object which returns true only for 'Published' state.
Suggested change
| // Filter out paused listings from search results for reservers | |
| // Paused listings should not be visible to reservers | |
| return allListings.filter(listing => listing.state !== 'Paused'); | |
| // Only show listings in 'Published' state to reservers in search results | |
| return allListings.filter(listing => listing.state === 'Published'); |
| await context.applicationServices.Listing.ItemListing.pause({ | ||
| id: args.id, | ||
| }); | ||
| return result; |
There was a problem hiding this comment.
Missing semicolon at the end of the return statement. This is inconsistent with the code style used in the cancelItemListing mutation above (line 160) and throughout the codebase.
| /** | ||
| * Maps domain listing state to UI status | ||
| * Domain states: Published, Paused, Cancelled, Drafted, Expired, Blocked, Appeal Requested | ||
| * UI statuses: Active, Paused, Cancelled, Draft, Expired, Blocked, Cancelled |
There was a problem hiding this comment.
The comment in line 16 lists "Cancelled" twice in the "UI statuses" list. It should only appear once.
Suggested change
| * UI statuses: Active, Paused, Cancelled, Draft, Expired, Blocked, Cancelled | |
| * UI statuses: Active, Paused, Cancelled, Draft, Expired, Blocked |
| onAction: (action: string, listingId: string) => { | ||
| if (action === 'pause') { | ||
| console.log('Pause action triggered for listing:', listingId); | ||
| alert(`Pausing listing ${listingId}. In real app, this would show a confirmation modal.`); |
There was a problem hiding this comment.
The alert message says "In real app, this would show a confirmation modal", but in the actual implementation (lines 62-73 in all-listings-table.tsx), a Popconfirm component is already used as the confirmation modal. This comment is misleading.
Consider updating the message to:
alert(`Listing ${listingId} paused. The Popconfirm modal would appear in the actual component.`);
Suggested change
| alert(`Pausing listing ${listingId}. In real app, this would show a confirmation modal.`); | |
| alert(`Listing ${listingId} paused. The Popconfirm modal would appear in the actual component.`); |
| success: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Error: ';' expected.
Suggested change
| }); | |
| }; |
| success: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Error: Declaration or statement expected.
Suggested change
| }); | |
| }; |
| success: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Error: Declaration or statement expected.
Suggested change
| }); | |
| }; |
…w build configurations and scripts.
Resolved conflicts in pnpm-lock.yaml: - Kept rimraf@^6.1.0 (security upgrade from feature branch) - Kept only glob@11.1.0 (removed glob@10.4.5 per feature branch security override)
Resolved conflicts in: - application-services/src/contexts/listing/item/index.ts * Merged pause, delete, update, and unblock methods - graphql/src/schema/types/listing/item-listing.graphql * Added pauseItemListing alongside cancelItemListing and deleteItemListing * Updated cancelItemListing and deleteItemListing to return ItemListingMutationResult - graphql/src/schema/types/listing/item-listing.resolvers.ts * Added pauseItemListing mutation resolver * Fixed cancelItemListing and deleteItemListing to return proper mutation result structure * Kept filtering of paused listings in itemListings query - graphql/src/schema/types/listing/item-listing.resolvers.test.ts * Merged mock context to include all methods (pause, deleteListings, unblock) * Added pause listing test scenarios - ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-table.container.graphql * Merged pauseItemListing, cancelItemListing, and deleteItemListing mutations - ui-sharethrift/src/components/layouts/home/my-listings/components/all-listings-table.container.tsx * Integrated pause, cancel, and delete mutations * Kept mapDomainStateToUIStatus helper for state mapping Regenerated GraphQL types via pnpm run gen
- Removed unused mapDomainStateToUIStatus function causing TS6133 error - Function was not being used anywhere in the component - Fixes Azure Pipeline build failure
- Inline immediately returned variable in pauseItemListing resolver - Add ownership verification to pause listing functionality - Only the listing owner (sharer) can pause their listing - Prevents unauthorized users from pausing listings - Optimize paused listing filtering by pushing to database layer - Move filter from in-memory to MongoDB query using $nin operator - Improves performance by avoiding loading all listings into memory - Add excludeStates parameter to ItemListingQueryAllCommand Security: Implements authorization check for pause action Performance: Database-level filtering instead of application-level
…atures - Update queryAll expectations to include excludeStates: ['Paused'] - Update pause expectations to include userEmail parameter - Fixes tests after implementing Sourcery code review suggestions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Implements pause listing functionality for sharers, allowing them to pause active listings which removes them from search results while retaining reservation history.
Changes
pause.ts) following cancel patternpauseItemListingGraphQL mutation and resolveritemListingsqueryImplementation Details
Testing
Related
Closes #42
Checklist
Summary by Sourcery
Enable sharers to pause active listings by adding backend service, GraphQL mutation, and UI controls while filtering paused listings from search results and standardizing status display.
New Features:
Bug Fixes:
Enhancements: