Add pickup point selection to checkout for pickup_point delivery#425
Add pickup point selection to checkout for pickup_point delivery#425CherryCIC wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughReplace InPost locker-based delivery with generic pickup-point support across data models, repository, view model state and fetching, order generation, UI widgets, and tests. Adds PickupPoint model with JSON deserialization, delivery choice constants, repository fetchPickupPoints method, async pickup-point fetching with address-based lookup and status management, and refactored delivery UI using a bottom-sheet picker. ChangesCheckout Pickup Point Migration
Sequence Diagram(s)sequenceDiagram
participant User
participant DeliveryOptions
participant ViewModel as CheckoutViewModel
participant Repo as CheckoutRepository
participant Sheet as PickupPointPickerSheet
participant Backend
User->>DeliveryOptions: Select pickup delivery
DeliveryOptions->>DeliveryOptions: _selectPickupDelivery()
DeliveryOptions->>ViewModel: setDeliveryChoice('pickup_point')
DeliveryOptions->>ViewModel: fetchPickupPointsForShippingAddress()
ViewModel->>Repo: fetchPickupPoints(country, address, radius)
Repo->>Backend: GET /shipping/pickup-points?country=...&address=...&radius=5000
Backend-->>Repo: List of PickupPoint JSON objects
Repo-->>ViewModel: Result.success(response)
ViewModel->>ViewModel: _parsePickupPointList(response)
ViewModel-->>DeliveryOptions: Status updated, pickupPoints populated
DeliveryOptions->>Sheet: showModalBottomSheet(pickupPoints)
User->>Sheet: Select a pickup point
Sheet-->>DeliveryOptions: Selected PickupPoint
DeliveryOptions->>ViewModel: setSelectedPickupPoint(point)
DeliveryOptions->>User: Display selected point details
sequenceDiagram
participant User
participant CheckoutUI as Checkout Page
participant ViewModel as CheckoutViewModel
participant Repo as CheckoutRepository
participant Firestore
participant OrderBackend as Order Backend
User->>CheckoutUI: Tap Pay button
CheckoutUI->>ViewModel: processCheckout()
ViewModel->>ViewModel: createOrder() with deliveryMethod + pickupPoint
ViewModel->>Firestore: storeOrderInFirestore(order with delivery info)
Firestore-->>ViewModel: Document stored
ViewModel->>OrderBackend: POST /orders with deliveryMethod and pickupPoint
OrderBackend-->>ViewModel: Order confirmation
ViewModel->>CheckoutUI: notifyListeners() → _handleOrderStatus
CheckoutUI->>User: Order success / navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02506995dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int radius = AddressConstants.pickupPointSearchRadiusMeters, | ||
| }) async { | ||
| _status = Status.loading; | ||
| selectedPickupPoint = null; |
There was a problem hiding this comment.
Clearing selectedPickupPoint at the start of fetchPickupPoints causes a regression where an already-valid pickup selection is lost before new data is available. If the refresh request fails or returns an empty payload (for example, transient network/API issues after the user taps pickup again), checkout becomes blocked even though the previous pickup point was still usable. Keep the existing selection until the new list is successfully loaded and only clear it if it is no longer present in the refreshed results.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/checkout/checkout_view_model.dart (1)
507-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify delivery method key in order payloads.
createOrder()sendsdeliveryMethodwhile other checkout payload builders usedelivery_method. This contract drift can cause delivery mode to be dropped/misread downstream.Proposed fix
- "deliveryMethod": deliveryChoice, + "delivery_method": deliveryChoice,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/checkout/checkout_view_model.dart` around lines 507 - 514, The order payload in createOrder() uses the key "deliveryMethod" but other checkout payload builders expect "delivery_method", causing a contract mismatch; update the Map<String, dynamic> orderData construction (the orderData variable in createOrder()) to use "delivery_method": deliveryChoice instead of "deliveryMethod" so the payload aligns with other builders and downstream consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/checkout/checkout_repository.dart`:
- Line 50: The fallback error message inside fetchNearestInPosts uses a generic
"Pickup points..." text which is misleading for locker flows; update the
fallback used with result.error (the expression result.error ?? '...') to a
locker-specific message such as "Lockers currently unavailable, please try again
later" so UX and diagnostics correctly reflect the locker path.
- Around line 11-15: Update the fetchPickupPoints interface declaration so its
radius parameter matches the implementation: make radius a named parameter with
a default value (int radius = AddressConstants.pickupPointSearchRadiusMeters)
instead of an optional nullable int; change the signature of Future<Result>
fetchPickupPoints({ required String country, required String address, int
radius, }); to use the defaulted radius to align with the concrete
implementation.
In `@lib/features/checkout/models/pickup_point.dart`:
- Line 74: The pickup-point payload currently hardcodes 'inpost_gb' when carrier
is missing; update the serialization in the PickupPoint model (the carrier
field/ toJson/map logic) to not inject a legacy provider: replace the ternary
that returns 'inpost_gb' with a nullable/omitted value (e.g. set carrier to null
or skip the 'carrier' key when carrier is null/empty), e.g. change the
expression for 'carrier' in PickupPoint.toJson (or the map builder) from
returning 'inpost_gb' to returning null or not emitting the key, and ensure
callers that consume PickupPoint handle a nullable/missing carrier.
In `@lib/features/checkout/widgets/delivery_options.dart`:
- Line 303: Replace the hardcoded UI strings used in the delivery options widget
with localized entries from AppStrings: change the
Text('${model.pickupPoints.length} pick-up points nearby') and the 'Open this
week' label in the same widget to use AppStrings (e.g.,
AppStrings.pickUpPointsNearby and AppStrings.openThisWeek) and interpolate the
pickupPoints length into the localized template; if those keys do not exist yet,
add parameterized entries to AppStrings (with a placeholder for the count) and
update any usages to call the localization accessor (e.g.,
AppStrings.pickUpPointsNearby(count)) so all user-visible text in the widget
uses the l10n layer.
- Around line 286-293: The tap and button handlers call the async callback
onChoose() without awaiting it (in the anonymous closures used for onPressed and
onTap), so convert those closures to async and await onChoose() to ensure errors
are captured and ordering is preserved; locate the anonymous handlers
referencing onChoose (the onPressed: () { onChoose(); } and onTap: () {
onChoose(); } closures in delivery_options.dart) and change them to async
closures that await onChoose() (e.g., onPressed: () async { await onChoose(); }
and onTap: () async { await onChoose(); }).
In `@lib/features/checkout/widgets/shipping_list_item.dart`:
- Around line 32-35: The trailing Icon used to reflect selection state
(isSelected) lacks accessibility semantics; update the ListTile trailing so
screen readers understand this is a mutually-exclusive radio selection — either
replace the Icon with Flutter's Radio widget bound to isSelected and the same
onTap handler (keeping ListTile and onTap behavior), or wrap the existing Icon
with a Semantics widget and set checked: isSelected and
inMutuallyExclusiveGroup: true (also provide a label if appropriate) so
assistive tech correctly announces selection state and group behavior.
---
Outside diff comments:
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 507-514: The order payload in createOrder() uses the key
"deliveryMethod" but other checkout payload builders expect "delivery_method",
causing a contract mismatch; update the Map<String, dynamic> orderData
construction (the orderData variable in createOrder()) to use "delivery_method":
deliveryChoice instead of "deliveryMethod" so the payload aligns with other
builders and downstream consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6415df9-5026-46bc-95f0-d3a22f5118f2
📒 Files selected for processing (11)
lib/core/config/app_strings.dartlib/core/services/network/api_endpoints.dartlib/features/checkout/checkout_page.dartlib/features/checkout/checkout_repository.dartlib/features/checkout/checkout_view_model.dartlib/features/checkout/constants/address_constants.dartlib/features/checkout/models/pickup_point.dartlib/features/checkout/widgets/delivery_options.dartlib/features/checkout/widgets/share_location_dialog.dartlib/features/checkout/widgets/shipping_list_item.darttest/checkout_view_model_test.dart
💤 Files with no reviewable changes (1)
- lib/features/checkout/widgets/share_location_dialog.dart
| Future<Result> fetchPickupPoints({ | ||
| required String country, | ||
| required String address, | ||
| int radius, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Dart null safety, is an optional named parameter like {int radius}valid withoutrequired or a default value?
💡 Result:
In Dart with null safety, an optional named parameter like {int radius} is not valid because its type is non-nullable [1][2]. When a parameter is optional and no default value is provided, Dart implicitly assigns it a default value of null [1][2]. If the parameter's type is non-nullable (such as int), this causes a compile-time error because the implicit null value conflicts with the non-nullable type [1][2]. To make such a parameter valid, you must choose one of the following approaches [3][1]: 1. Make the type nullable: Use {int? radius}. This explicitly allows null to be the default value [3][1]. 2. Provide a default value: Use {int radius = 0} (or another appropriate default). This ensures the parameter will have a non-null value if omitted [3][1][2]. 3. Mark it as required: Use {required int radius}. This removes the optionality, forcing the caller to provide the argument [3][1].
Citations:
- 1: https://dart.dev/null-safety/understanding-null-safety
- 2: https://stackoverflow.com/questions/64560461/the-parameter-cant-have-a-value-of-null-because-of-its-type-in-dart
- 3: how to use optional named parameters in NULL SAFETY? dart-lang/sdk#46128
🏁 Script executed:
# Check the actual file content around lines 11-15
cat -n lib/features/checkout/checkout_repository.dart | head -20Repository: Cherry-CIC/MVP
Length of output: 1021
🏁 Script executed:
# Find the implementation of fetchPickupPoints to see what default is used
rg -A 10 "fetchPickupPoints" lib/features/checkout/ -g "*.dart"Repository: Cherry-CIC/MVP
Length of output: 8358
🏁 Script executed:
# Find AddressConstants to verify the constant exists
fd -t f "AddressConstants" lib/
rg "pickupPointSearchRadiusMeters" --type dartRepository: Cherry-CIC/MVP
Length of output: 506
Fix the radius parameter declaration in the interface to match the implementation.
The interface declares int radius as an optional named parameter without required or a default value, which violates Dart null-safety. The concrete implementation correctly provides int radius = AddressConstants.pickupPointSearchRadiusMeters as default. Align the interface signature with the implementation to resolve the compile-time error.
Proposed fix
Future<Result> fetchPickupPoints({
required String country,
required String address,
- int radius,
+ int radius = AddressConstants.pickupPointSearchRadiusMeters,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<Result> fetchPickupPoints({ | |
| required String country, | |
| required String address, | |
| int radius, | |
| }); | |
| Future<Result> fetchPickupPoints({ | |
| required String country, | |
| required String address, | |
| int radius = AddressConstants.pickupPointSearchRadiusMeters, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/checkout/checkout_repository.dart` around lines 11 - 15, Update
the fetchPickupPoints interface declaration so its radius parameter matches the
implementation: make radius a named parameter with a default value (int radius =
AddressConstants.pickupPointSearchRadiusMeters) instead of an optional nullable
int; change the signature of Future<Result> fetchPickupPoints({ required String
country, required String address, int radius, }); to use the defaulted radius to
align with the concrete implementation.
| return Result.failure( | ||
| result.error ?? | ||
| 'Pickup points currently unavailable, please try again later', | ||
| result.error ?? 'Pickup points currently unavailable, please try again later', |
There was a problem hiding this comment.
Use a locker-specific fallback message in fetchNearestInPosts.
The current fallback says “Pickup points…” inside the locker fetch path, which is misleading for UX and diagnostics.
Proposed fix
- result.error ?? 'Pickup points currently unavailable, please try again later',
+ result.error ?? 'Pickup lockers currently unavailable, please try again later',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result.error ?? 'Pickup points currently unavailable, please try again later', | |
| result.error ?? 'Pickup lockers currently unavailable, please try again later', |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/checkout/checkout_repository.dart` at line 50, The fallback
error message inside fetchNearestInPosts uses a generic "Pickup points..." text
which is misleading for locker flows; update the fallback used with result.error
(the expression result.error ?? '...') to a locker-specific message such as
"Lockers currently unavailable, please try again later" so UX and diagnostics
correctly reflect the locker path.
| 'city': city, | ||
| 'postalCode': postalCode, | ||
| 'country': country, | ||
| 'carrier': carrier == null || carrier!.isEmpty ? 'inpost_gb' : carrier, |
There was a problem hiding this comment.
Avoid hard-coding a legacy carrier in generic pickup-point payloads.
Defaulting carrier to inpost_gb injects provider-specific data even when no carrier was returned/selected, which can produce incorrect order payloads.
Proposed fix
- 'carrier': carrier == null || carrier!.isEmpty ? 'inpost_gb' : carrier,
+ if (carrier != null && carrier!.isNotEmpty) 'carrier': carrier,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'carrier': carrier == null || carrier!.isEmpty ? 'inpost_gb' : carrier, | |
| if (carrier != null && carrier!.isNotEmpty) 'carrier': carrier, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/checkout/models/pickup_point.dart` at line 74, The pickup-point
payload currently hardcodes 'inpost_gb' when carrier is missing; update the
serialization in the PickupPoint model (the carrier field/ toJson/map logic) to
not inject a legacy provider: replace the ternary that returns 'inpost_gb' with
a nullable/omitted value (e.g. set carrier to null or skip the 'carrier' key
when carrier is null/empty), e.g. change the expression for 'carrier' in
PickupPoint.toJson (or the map builder) from returning 'inpost_gb' to returning
null or not emitting the key, and ensure callers that consume PickupPoint handle
a nullable/missing carrier.
| onPressed: () { | ||
| onChoose(); | ||
| }, | ||
| child: const Text(AppStrings.checkoutChangePickupPoint), | ||
| ), | ||
| onTap: () { | ||
| onChoose(); | ||
| }, |
There was a problem hiding this comment.
Await async pickup actions in tap handlers.
At Line 286 and Line 305, onChoose() is invoked without awaiting its Future<void>, so async errors can escape the handler and be harder to control. Make these handlers async and await onChoose().
Suggested fix
- trailing: TextButton(
- onPressed: () {
- onChoose();
- },
+ trailing: TextButton(
+ onPressed: () async {
+ await onChoose();
+ },
child: const Text(AppStrings.checkoutChangePickupPoint),
),
- onTap: () {
- onChoose();
- },
+ onTap: () async {
+ await onChoose();
+ },
...
- onTap: () {
- onChoose();
- },
+ onTap: () async {
+ await onChoose();
+ },Also applies to: 305-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/checkout/widgets/delivery_options.dart` around lines 286 - 293,
The tap and button handlers call the async callback onChoose() without awaiting
it (in the anonymous closures used for onPressed and onTap), so convert those
closures to async and await onChoose() to ensure errors are captured and
ordering is preserved; locate the anonymous handlers referencing onChoose (the
onPressed: () { onChoose(); } and onTap: () { onChoose(); } closures in
delivery_options.dart) and change them to async closures that await onChoose()
(e.g., onPressed: () async { await onChoose(); } and onTap: () async { await
onChoose(); }).
| contentPadding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8), | ||
| leading: const Icon(Icons.map_outlined), | ||
| title: const Text(AppStrings.checkoutChoosePickupPoint), | ||
| subtitle: model.pickupPoints.isEmpty ? null : Text('${model.pickupPoints.length} pick-up points nearby'), |
There was a problem hiding this comment.
Localize newly added user-visible strings.
At Line 303 and Line 449, UI text is hardcoded ("pick-up points nearby", "Open this week"). These should come from AppStrings to avoid l10n regressions.
Also applies to: 449-449
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/checkout/widgets/delivery_options.dart` at line 303, Replace the
hardcoded UI strings used in the delivery options widget with localized entries
from AppStrings: change the Text('${model.pickupPoints.length} pick-up points
nearby') and the 'Open this week' label in the same widget to use AppStrings
(e.g., AppStrings.pickUpPointsNearby and AppStrings.openThisWeek) and
interpolate the pickupPoints length into the localized template; if those keys
do not exist yet, add parameterized entries to AppStrings (with a placeholder
for the count) and update any usages to call the localization accessor (e.g.,
AppStrings.pickUpPointsNearby(count)) so all user-visible text in the widget
uses the l10n layer.
| trailing: Icon( | ||
| isSelected ? Icons.radio_button_checked : Icons.radio_button_unchecked, | ||
| color: isSelected ? Theme.of(context).colorScheme.primary : Theme.of(context).colorScheme.onSurfaceVariant, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its contents
fd "shipping_list_item.dart"Repository: Cherry-CIC/MVP
Length of output: 112
🏁 Script executed:
# Read the file to see the code context
cat -n lib/features/checkout/widgets/shipping_list_item.dartRepository: Cherry-CIC/MVP
Length of output: 1545
🏁 Script executed:
# Check git history for this file to see if it was previously using Radio
git log --oneline -n 20 lib/features/checkout/widgets/shipping_list_item.dartRepository: Cherry-CIC/MVP
Length of output: 105
🏁 Script executed:
# Check the diff of the commit to see what changed
git show 0250699:lib/features/checkout/widgets/shipping_list_item.dartRepository: Cherry-CIC/MVP
Length of output: 1258
🏁 Script executed:
# Let me check if there are other similar radio selection patterns in the codebase
rg -t dart "Radio\s*\(" --max-count 20Repository: Cherry-CIC/MVP
Length of output: 40
🏁 Script executed:
# Check if there's a commit before this one that might show the previous state
git log --oneline --all | head -20Repository: Cherry-CIC/MVP
Length of output: 1381
🏁 Script executed:
# Check if this is a new file or if it was changed in the commit
git show --name-status 0250699Repository: Cherry-CIC/MVP
Length of output: 18066
🏁 Script executed:
# Check Flutter's ListTile semantics in Material Design context
# Search for any Semantics wrappers or similar patterns in the codebase
rg -t dart "Semantics\s*\(" lib/features/checkout/ -A 3Repository: Cherry-CIC/MVP
Length of output: 40
Add semantic context for radio-button selection pattern.
This widget uses Icon to display radio button state but lacks semantic markup for screen readers. The ListTile's onTap provides interaction, but assistive tech won't understand this represents a mutually-exclusive group selection. Either use the Radio widget instead, or wrap the Icon in a Semantics widget with checked: isSelected and inMutuallyExclusiveGroup: true so screen readers correctly announce the selection state and group behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/checkout/widgets/shipping_list_item.dart` around lines 32 - 35,
The trailing Icon used to reflect selection state (isSelected) lacks
accessibility semantics; update the ListTile trailing so screen readers
understand this is a mutually-exclusive radio selection — either replace the
Icon with Flutter's Radio widget bound to isSelected and the same onTap handler
(keeping ListTile and onTap behavior), or wrap the existing Icon with a
Semantics widget and set checked: isSelected and inMutuallyExclusiveGroup: true
(also provide a label if appropriate) so assistive tech correctly announces
selection state and group behavior.
1 participant
Summary
GET /api/shipping/pickup-pointsusing checkout address data and the documented default radius.PickupPointmodel and updated checkout state to store the selected pickup point alongside the delivery method.pickup_pointdelivery until a pickup point is selected.pickup_point.Testing
flutter test test/checkout_view_model_test.dartpassed.dart analyze lib/features/checkout test/checkout_view_model_test.dartpassed with only existing deprecation infos in unrelated checkout payment widgets.flutter analyzestill reports unrelated pre-existing test mock issues intest/auth_ui_test.dart, outside this change.Notes
lib/features/checkout/widgets/share_location_dialog.dartwas removed because the new flow no longer uses the old postcode prompt.The frontend can now select and submit a pick-up point, but the final order payload may still fail backend shipment creation because required Sendcloud/payment fields may need work.
Summary by CodeRabbit
New Features
Improvements