Fixes: Closes #335#371
Conversation
… CourierModel and PickupPointModel- Refactored CheckoutViewModel to support courier selection and payload generation- Updated delivery options UI to allow selecting couriers and their respective pickup points- Added payment validation for pickup point requirements- Removed legacy InPost hardcoding
|
Hey! Just an FYI on a few minor project-level files that got updated alongside the checkout feature: While testing locally, the Android build was failing due to some older configurations. To get things compiling smoothly, I included a few small bumps: Updated the Android Gradle Plugin (AGP) version in Bumped the Gradle wrapper in Added a quick null-check fallback for flutter.ndkVersion in |
|
Hey @Z3DDIEZ thank you for this PR and for your contribution. I am personally grateful for people like you, willing to put in the legwork and support us in our mission. This PR is technically mergeable but has several important considerations you should address before merging... Here's a detailed assessment: Blocked is fine as this is due to the PR requiring an approval :) ✅ Strengths
|
|
Don't fret, I'm here to help as much as I can. I wanted to know, backend related changes and the qa tests remaining to be done, I don't know if that's beyond the scope of the pr, or not, but I do hope all is well. If not, feel free to let me know if there's anything else I may need to work on for this issue. |
MattGoodwin0
left a comment
MattGoodwin0
left a comment
There was a problem hiding this comment.
If you wouldn't mind just pulling out those three gradle changes, that would be great!
Rest of the PR looks perfect, thank you for that.
I think it would be safer to make gradle changes separately.
Done! I've reverted the gradle bumps so this PR is strictly focused on the pickup point logic. |
There was a problem hiding this comment.
5 new files modified/added:
-
New Models (clean, well-structured):
CourierModel- Simple data class with id, name, JSON serializationPickupPointModel- Extended from InpostModel with courier linking
-
CheckoutViewModel - Properly refactored:
- Added courier/pickup point state management
- Maintains backward compatibility with legacy
selectedInpost - Both new and old methods sync state correctly
- Proper payload mapping with
deliveryMethod,courier,pickupPointId - Cleaned up hardcoded test data
-
DeliveryOptions UI Widget - Good flow:
- Step 1: Show courier list
- Step 2: Show pickup points filtered by selected courier
- Step 3: Show selected point summary
- Fixed null safety issues (
?.isNotEmpty == true) - Removed problematic postcode requirement
-
CheckoutPage Validation - Updated correctly:
- Pay button now checks
selectedPickupPointinstead ofselectedInpost - Properly blocks checkout if no pickup point selected
- Pay button now checks
-
Tests Complete:
- All 5 scenarios verified with screenshots
- Gradle changes were reverted (as you requested)
Why It's Safe
✅ Scope-focused - Only touches checkout delivery flow
✅ Backward compatible - Legacy methods still work
✅ State management solid - Proper null resets when changing couriers
✅ Payload correct - Backend-ready delivery details included
✅ Validation enforced - Can't pay without selecting a point
✅ No breaking changes - Only additions and refactoring
From what I can tell... I can approve it. 👍
Thank you.
|
@MattGoodwin0 do you believe in your professional opinion that the requested changes have been met? |
fion21
left a comment
fion21
left a comment
There was a problem hiding this comment.
add this to your codel.yml file, as it s in your repo I dont have access t o it
_____ delete this line above
name: "CodeQL"
on:
push:
branches: [ "main" ]
pull_request:
branches: [ "main" ]
schedule:
- cron: '0 3 * * *'
env:
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true
jobs:
analyze:
name: Analyze
runs-on: ubuntu-latest
permissions:
security-events: write
actions: read
contents: read
steps:
- name: Checkout repository
uses: actions/checkout@v5
- name: Initialize CodeQL
uses: github/codeql-action/init@v4
with:
languages: actions
- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v4
|
@CherryCIC done as instructed! |
|
All 4 conflicting files in PR #371 ("Fixes: Closes #335") have been successfully resolved and the merge commit has been committed. Here's a summary of the decisions made for each file: [github](#371) 1.
2.
3.
4.
The PR now has no merge conflicts and is ready for review/merge. It preserves the full "Ship to pick-up point" feature including courier selection, pickup point listing, and pay button validation. [github](#371) I think it is ok to merge now @Z3DDIEZ @fion21 @MattGoodwin0 thanks for your support on this guys, another quick opinion if you can would be much appreciated for this one before merge. |
CherryCIC
left a comment
CherryCIC
left a comment
There was a problem hiding this comment.
I approve this PR after resolving the conflicts, but would value another opinion/ review before merging.
|
Sorry to overwhelm you @JaseElder if I am but can I get a second review on this please so we can merge it? Moderate priority on this one. |
|
No worries!
I'll try to get on to these in the morning. Will you be around to have a
quick chat about the other?
…On Thu, 9 Apr 2026, 20:03 cherry, ***@***.***> wrote:
*CherryCIC* left a comment (Cherry-CIC/MVP#371)
<#371 (comment)>
Sorry to overwhelm you @JaseElder <https://github.com/JaseElder> if I am
but can I get a second review on this please so we can merge it? Moderate
priority on this one.
—
Reply to this email directly, view it on GitHub
<#371 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEQXDRCKA7HTHN7M3AOGH34U7XYPAVCNFSM6AAAAACV554WHKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJWG43TSNJWGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thanks Jason. I will be around throughout the day. Give me a call at a time that feels convenient for you and if I can't catch you I'll call you back. @JaseElder |
The refactor to _nearestPickupPoints removed the _nearestInpost declaration, but it was still referenced in fetchNearestPickupPoints() (lines 387, 411) and by checkout_view_model_test.dart. This restores the field and its public getter to fix the compile error and unbreak the test suite. Addresses review feedback from @JaseElder.
JaseElder
left a comment
JaseElder
left a comment
There was a problem hiding this comment.
Found a couple of issues stopping the app from compiling.
There was a problem hiding this comment.
isLoading isn't declared anywhere.
| AppStrings.checkoutPaymentMethodRequired; | ||
| }); | ||
|
|
||
| await showModalBottomSheet<PaymentType>( |
There was a problem hiding this comment.
Missing an import for PaymentType
There was a problem hiding this comment.
Good day, fixed via my latest commit. Do let me know if theres anything else you would like me to handle!
|
@Z3DDIEZ Hi mate. Would you like any support with this? |
|
@CherryCIC I would love some, currently I'm indisposed and very far from any computers, unavailable for at least 2 weeks, heading somewhere remote. |
|
Where would you like support? |
|
I have resolved the core issues blocking the merge. This PR adds the “Ship to pick-up point” checkout option for #335, replacing hard-coded InPost locker logic with a courier and pickup point selection flow. It appears to meet the acceptance criteria: users can choose pickup delivery, select a courier and location, see a summary, are blocked from paying until a pickup point is selected, and the order payload includes the delivery method, courier, and pickup point ID. However, it is not merge-ready because reviewers have flagged compilation issues, plus there may be unrelated Gradle/CodeQL changes inherited from other work. Next step: fix the build errors, strip out unrelated changes, confirm the backend payload shape, then re-request review. |
|
@CherryCIC I was away, deep in the mountains, away from all forms of tech and network connection. |
- Add missing import for PaymentType enum used in showModalBottomSheet - Replace undeclared `isLoading` variable with `viewModel.createOrderStatus.type == StatusType.loading` Addresses review feedback from JaseElder in PR Cherry-CIC#371.
|
Amazing news @Z3DDIEZ ! I hope you had a nice time. Where abouts did you go? |
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis PR refactors the checkout delivery flow to support courier-managed pickup points. New data models ( ChangesCourier and Pickup Point Selection Flow
Sequence Diagram(s)The changes introduce a new multi-step courier-driven pickup point selection process. Here is the observable interaction: sequenceDiagram
participant User
participant DeliveryOptions as DeliveryOptions Widget
participant ViewModel as CheckoutViewModel
participant API as Pickup Points API
User->>DeliveryOptions: select Pickup delivery
DeliveryOptions->>ViewModel: show couriers list
User->>DeliveryOptions: select courier
DeliveryOptions->>ViewModel: setSelectedCourier()
ViewModel->>API: fetchNearestPickupPoints(postal code)
API-->>ViewModel: nearby points for courier
DeliveryOptions->>ViewModel: show nearest pickup points
User->>DeliveryOptions: select pickup point
DeliveryOptions->>ViewModel: setSelectedPickupPoint()
ViewModel->>ViewModel: updateDeliveryDetails()
User->>DeliveryOptions: confirm & pay
ViewModel->>ViewModel: createOrder(deliveryDetails)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (2)
.github/workflows/codeql.yml (1)
1-34: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove unrelated CodeQL workflow changes from this PR.
This CodeQL workflow modification is unrelated to the "Ship to pick-up point" checkout feature implementation. Mixing infrastructure/CI changes with feature changes makes the PR harder to review, increases merge risk, and violates single-responsibility principles for pull requests.
As noted in the review comments, unrelated changes should be stripped before merging. If the CodeQL/Node24 changes are needed, they should be submitted in a separate PR with appropriate context and testing.
♻️ Recommendation
Revert all changes to
.github/workflows/codeql.ymlin this PR. If the workflow updates are necessary, create a separate PR specifically for CI/workflow improvements with:
- Clear justification for the Node24 requirement
- Evidence that the change doesn't break existing CodeQL scans
- Appropriate reviewers with CI/infrastructure context
🤖 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 @.github/workflows/codeql.yml around lines 1 - 34, This PR inadvertently includes unrelated CI changes to the CodeQL workflow (.github/workflows/codeql.yml); revert the edits so the file returns to its previous state (remove the FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env addition and any changes to the analyze job such as the languages: actions block or altered action versions like github/codeql-action/init@v4), and if Node24 or other CodeQL updates are actually required, open a separate PR with justification and tests rather than bundling those changes into the "Ship to pick-up point" feature PR.lib/features/checkout/checkout_view_model.dart (1)
374-413:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
fetchNearestPickupPointsdoes not populatenearestPickupPoints.This method fetches data and populates
_nearestInpost, but thenearestPickupPointsgetter (used by the UI) returns from the hardcoded_nearestPickupPointslist. The API results are never surfaced to the new courier-based UI flow.To connect the data flow, the parsed results should update
_nearestPickupPoints(with appropriatecourierIdmapping), or the getter should read from_nearestInpostwith conversion.🤖 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 374 - 413, fetchNearestPickupPoints populates _nearestInpost but never updates the _nearestPickupPoints used by the UI; update the parsed results into _nearestPickupPoints (or change the nearestPickupPoints getter to return a mapped view of _nearestInpost). Specifically, inside fetchNearestPickupPoints after parsing (where parsedInposts is created) map each InpostModel to your pickup-point representation (assign the appropriate courierId) and replace _nearestPickupPoints contents (clear/addAll mapped list) so the UI-facing nearestPickupPoints reflects API results; ensure any code that relies on courierId uses the mapped value.
🧹 Nitpick comments (2)
lib/features/checkout/checkout_view_model.dart (1)
38-87: 💤 Low valueHardcoded mock data is not production-ready.
The
_couriersand_nearestPickupPointslists are hardcoded. This is acceptable for development/testing but should be replaced with real API data before production. Consider adding a TODO comment or tracking issue.Do you want me to open an issue to track the backend integration for couriers and pickup points?
🤖 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 38 - 87, Replace the hardcoded mock lists with production data: remove or mark the hardcoded _couriers and _nearestPickupPoints and add a TODO plus a plan to fetch real data (e.g., via an injected repository/service) used by the couriers getter and the nearestPickupPoints getter (which currently filters by selectedCourier); implement async methods like fetchCouriers() and fetchPickupPoints() that populate those private lists (or expose streams) and update selectedCourier handling to work with the real data source; include a clear TODO comment referencing backend integration so this change is tracked.lib/features/checkout/widgets/delivery_options.dart (1)
4-4: ⚡ Quick winUnused import.
fluttertoastis imported but no longer used in this file after the refactor removed the postcode-required toast.🧹 Remove unused import
-import 'package:fluttertoast/fluttertoast.dart';🤖 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 4, Remove the unused import of fluttertoast: delete the import line "import 'package:fluttertoast/fluttertoast.dart';" from the top of delivery_options.dart (the leftover from the removed postcode-required toast) so there are no unused imports in the file and the analyzer warning disappears.
🤖 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/core/models/courier_model.dart`:
- Around line 10-15: The CourierModel.fromJson factory currently uses unsafe
casts (id: json['id'] as String, name: json['name'] as String); update it to
defensively parse and validate types: check that json contains keys and that
values are non-null strings (e.g., use runtime type checks or toString
fallbacks) before assigning to the CourierModel constructor, and provide
sensible defaults or throw a clear FormatException when values are
missing/invalid so malformed JSON cannot cause a runtime cast error.
In `@lib/core/models/pickup_point_model.dart`:
- Around line 20-30: The fromJson factory (PickupPointModel.fromJson) currently
uses unsafe casts like json['id'] as String for fields id, name, address,
postcode, lat, long, and courierId which can throw if values are null or not
strings; update this method to defensively parse each value (e.g., check for
null and type, use json['field'] is String ? json['field'] as String : fallback
or convert toString() where appropriate) and provide sensible defaults or throw
a clear FormatException with context if required so missing/invalid backend
values don't cause uncaught TypeErrors at runtime.
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 443-451: The selectedPickupPoint is being hydrated with a
hardcoded courierId ('inpost') which can misrepresent stored data; update the
hydration in the code that builds PickupPointModel (the selectedPickupPoint
assignment) to read courierId from the Firestore document (use
doc.get(FirestoreConstants.courierId) or a safe default if missing) so
PickupPointModel.courierId reflects the stored value instead of always 'inpost'.
- Around line 585-594: The deliveryDetails construction uses fallback 'unknown'
values when deliveryChoice == 'pickup' but selectedPickupPoint can be null; add
a defensive guard before building deliveryDetails: check deliveryChoice ==
'pickup' and if selectedPickupPoint == null then fail fast (throw an exception,
return an error, or log and abort submission) so you never send courier:
'unknown' or pickupPointId: 'unknown'; update the function that builds/returns
deliveryDetails (referenced by deliveryChoice and selectedPickupPoint) to
perform this validation and handle the error path appropriately.
In `@lib/features/checkout/widgets/delivery_options.dart`:
- Line 298: The branch treating StatusType.uninitialized the same as
StatusType.success causes mock pickup content to render before a fetch; update
the conditional in the widget that checks status.type so it only treats
StatusType.success as the "show results" path (i.e., replace the combined check
in the clause that currently reads "status.type == StatusType.success ||
status.type == StatusType.uninitialized" with a single check for
StatusType.success), and add an explicit branch for StatusType.uninitialized
that renders a prompt/placeholder asking the user to enter a postcode (or
otherwise indicates no data fetched yet) so the UI does not display pickup
lists/points prematurely.
- Around line 356-361: The UI is still bound to the hardcoded
_nearestPickupPoints instead of the API result stored in _nearestInpost, so
tapping a courier updates model._nearestInpost via fetchNearestPickupPoints but
the widgets never read it; update the data flow so the ListTile tap path
(setSelectedCourier, fetchNearestPickupPoints) results are displayed by wiring
the UI to the model.nearestInpost (or merging _nearestInpost into the existing
PickupPointModel list) and ensure showLocker is used to choose which list to
render; specifically change the widgets that currently read _nearestPickupPoints
to prefer model.nearestInpost when non-empty (or after successful fetch), and
keep setSelectedCourier, fetchNearestPickupPoints, nearestInpost,
_nearestInpost, _nearestPickupPoints and showLocker consistent so the fetched
points replace or augment the mock list.
---
Outside diff comments:
In @.github/workflows/codeql.yml:
- Around line 1-34: This PR inadvertently includes unrelated CI changes to the
CodeQL workflow (.github/workflows/codeql.yml); revert the edits so the file
returns to its previous state (remove the FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env
addition and any changes to the analyze job such as the languages: actions block
or altered action versions like github/codeql-action/init@v4), and if Node24 or
other CodeQL updates are actually required, open a separate PR with
justification and tests rather than bundling those changes into the "Ship to
pick-up point" feature PR.
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 374-413: fetchNearestPickupPoints populates _nearestInpost but
never updates the _nearestPickupPoints used by the UI; update the parsed results
into _nearestPickupPoints (or change the nearestPickupPoints getter to return a
mapped view of _nearestInpost). Specifically, inside fetchNearestPickupPoints
after parsing (where parsedInposts is created) map each InpostModel to your
pickup-point representation (assign the appropriate courierId) and replace
_nearestPickupPoints contents (clear/addAll mapped list) so the UI-facing
nearestPickupPoints reflects API results; ensure any code that relies on
courierId uses the mapped value.
---
Nitpick comments:
In `@lib/features/checkout/checkout_view_model.dart`:
- Around line 38-87: Replace the hardcoded mock lists with production data:
remove or mark the hardcoded _couriers and _nearestPickupPoints and add a TODO
plus a plan to fetch real data (e.g., via an injected repository/service) used
by the couriers getter and the nearestPickupPoints getter (which currently
filters by selectedCourier); implement async methods like fetchCouriers() and
fetchPickupPoints() that populate those private lists (or expose streams) and
update selectedCourier handling to work with the real data source; include a
clear TODO comment referencing backend integration so this change is tracked.
In `@lib/features/checkout/widgets/delivery_options.dart`:
- Line 4: Remove the unused import of fluttertoast: delete the import line
"import 'package:fluttertoast/fluttertoast.dart';" from the top of
delivery_options.dart (the leftover from the removed postcode-required toast) so
there are no unused imports in the file and the analyzer warning disappears.
🪄 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: 5762f0b4-5390-4782-b396-5598cadf641a
📒 Files selected for processing (6)
.github/workflows/codeql.ymllib/core/models/courier_model.dartlib/core/models/pickup_point_model.dartlib/features/checkout/checkout_page.dartlib/features/checkout/checkout_view_model.dartlib/features/checkout/widgets/delivery_options.dart
| factory CourierModel.fromJson(Map<String, dynamic> json) { | ||
| return CourierModel( | ||
| id: json['id'] as String, | ||
| name: json['name'] as String, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same unsafe casting issue as PickupPointModel.
Apply defensive parsing here as well to avoid runtime crashes from malformed JSON.
🛡️ Proposed fix
factory CourierModel.fromJson(Map<String, dynamic> json) {
return CourierModel(
- id: json['id'] as String,
- name: json['name'] as String,
+ id: json['id']?.toString() ?? '',
+ name: json['name']?.toString() ?? '',
);
}📝 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.
| factory CourierModel.fromJson(Map<String, dynamic> json) { | |
| return CourierModel( | |
| id: json['id'] as String, | |
| name: json['name'] as String, | |
| ); | |
| } | |
| factory CourierModel.fromJson(Map<String, dynamic> json) { | |
| return CourierModel( | |
| id: json['id']?.toString() ?? '', | |
| name: json['name']?.toString() ?? '', | |
| ); | |
| } |
🤖 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/core/models/courier_model.dart` around lines 10 - 15, The
CourierModel.fromJson factory currently uses unsafe casts (id: json['id'] as
String, name: json['name'] as String); update it to defensively parse and
validate types: check that json contains keys and that values are non-null
strings (e.g., use runtime type checks or toString fallbacks) before assigning
to the CourierModel constructor, and provide sensible defaults or throw a clear
FormatException when values are missing/invalid so malformed JSON cannot cause a
runtime cast error.
| factory PickupPointModel.fromJson(Map<String, dynamic> json) { | ||
| return PickupPointModel( | ||
| id: json['id'] as String, | ||
| name: json['name'] as String, | ||
| address: json['address'] as String, | ||
| postcode: json['postcode'] as String, | ||
| lat: json['lat'] as String, | ||
| long: json['long'] as String, | ||
| courierId: json['courierId'] as String, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unsafe JSON parsing may cause runtime crashes.
The fromJson factory uses direct as String casts without null checks. If the backend returns incomplete data or null values, this will throw a TypeError at runtime.
Consider defensive parsing with fallbacks or explicit null checks:
🛡️ Proposed defensive parsing
factory PickupPointModel.fromJson(Map<String, dynamic> json) {
return PickupPointModel(
- id: json['id'] as String,
- name: json['name'] as String,
- address: json['address'] as String,
- postcode: json['postcode'] as String,
- lat: json['lat'] as String,
- long: json['long'] as String,
- courierId: json['courierId'] as String,
+ id: json['id']?.toString() ?? '',
+ name: json['name']?.toString() ?? '',
+ address: json['address']?.toString() ?? '',
+ postcode: json['postcode']?.toString() ?? '',
+ lat: json['lat']?.toString() ?? '',
+ long: json['long']?.toString() ?? '',
+ courierId: json['courierId']?.toString() ?? '',
);
}📝 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.
| factory PickupPointModel.fromJson(Map<String, dynamic> json) { | |
| return PickupPointModel( | |
| id: json['id'] as String, | |
| name: json['name'] as String, | |
| address: json['address'] as String, | |
| postcode: json['postcode'] as String, | |
| lat: json['lat'] as String, | |
| long: json['long'] as String, | |
| courierId: json['courierId'] as String, | |
| ); | |
| } | |
| factory PickupPointModel.fromJson(Map<String, dynamic> json) { | |
| return PickupPointModel( | |
| id: json['id']?.toString() ?? '', | |
| name: json['name']?.toString() ?? '', | |
| address: json['address']?.toString() ?? '', | |
| postcode: json['postcode']?.toString() ?? '', | |
| lat: json['lat']?.toString() ?? '', | |
| long: json['long']?.toString() ?? '', | |
| courierId: json['courierId']?.toString() ?? '', | |
| ); | |
| } |
🤖 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/core/models/pickup_point_model.dart` around lines 20 - 30, The fromJson
factory (PickupPointModel.fromJson) currently uses unsafe casts like json['id']
as String for fields id, name, address, postcode, lat, long, and courierId which
can throw if values are null or not strings; update this method to defensively
parse each value (e.g., check for null and type, use json['field'] is String ?
json['field'] as String : fallback or convert toString() where appropriate) and
provide sensible defaults or throw a clear FormatException with context if
required so missing/invalid backend values don't cause uncaught TypeErrors at
runtime.
| selectedPickupPoint = PickupPointModel( | ||
| id: doc.get(FirestoreConstants.id), | ||
| name: doc.get(FirestoreConstants.name), | ||
| address: doc.get(FirestoreConstants.address), | ||
| postcode: doc.get(FirestoreConstants.postcode), | ||
| lat: doc.get(FirestoreConstants.lat), | ||
| long: doc.get(FirestoreConstants.long), | ||
| courierId: 'inpost', | ||
| ); |
There was a problem hiding this comment.
Hardcoded courierId: 'inpost' may be incorrect for non-InPost lockers.
When hydrating selectedPickupPoint from Firestore, courierId is hardcoded to 'inpost'. If the user previously selected a different courier's pickup point, this would assign the wrong courier.
Consider storing and retrieving courierId from Firestore:
🛡️ Suggested fix
selectedPickupPoint = PickupPointModel(
id: doc.get(FirestoreConstants.id),
name: doc.get(FirestoreConstants.name),
address: doc.get(FirestoreConstants.address),
postcode: doc.get(FirestoreConstants.postcode),
lat: doc.get(FirestoreConstants.lat),
long: doc.get(FirestoreConstants.long),
- courierId: 'inpost',
+ courierId: doc.data()?.containsKey('courierId') == true
+ ? doc.get('courierId')
+ : 'inpost',
);📝 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.
| selectedPickupPoint = PickupPointModel( | |
| id: doc.get(FirestoreConstants.id), | |
| name: doc.get(FirestoreConstants.name), | |
| address: doc.get(FirestoreConstants.address), | |
| postcode: doc.get(FirestoreConstants.postcode), | |
| lat: doc.get(FirestoreConstants.lat), | |
| long: doc.get(FirestoreConstants.long), | |
| courierId: 'inpost', | |
| ); | |
| selectedPickupPoint = PickupPointModel( | |
| id: doc.get(FirestoreConstants.id), | |
| name: doc.get(FirestoreConstants.name), | |
| address: doc.get(FirestoreConstants.address), | |
| postcode: doc.get(FirestoreConstants.postcode), | |
| lat: doc.get(FirestoreConstants.lat), | |
| long: doc.get(FirestoreConstants.long), | |
| courierId: doc.data()?.containsKey('courierId') == true | |
| ? doc.get('courierId') | |
| : 'inpost', | |
| ); |
🤖 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 443 - 451, The
selectedPickupPoint is being hydrated with a hardcoded courierId ('inpost')
which can misrepresent stored data; update the hydration in the code that builds
PickupPointModel (the selectedPickupPoint assignment) to read courierId from the
Firestore document (use doc.get(FirestoreConstants.courierId) or a safe default
if missing) so PickupPointModel.courierId reflects the stored value instead of
always 'inpost'.
| final Map<String, dynamic> deliveryDetails = deliveryChoice == 'pickup' | ||
| ? { | ||
| "deliveryMethod": "pickup_point", | ||
| "courier": selectedPickupPoint?.courierId ?? 'unknown', | ||
| "pickupPointId": selectedPickupPoint?.id ?? 'unknown', | ||
| "pickupPointName": selectedPickupPoint?.name ?? 'unknown', | ||
| } | ||
| : { | ||
| "deliveryMethod": "ship_to_home", | ||
| }; |
There was a problem hiding this comment.
'unknown' fallback values may reach the backend.
If selectedPickupPoint is null when deliveryChoice == 'pickup', the order payload will contain courier: 'unknown' and pickupPointId: 'unknown'. While the UI should prevent this, adding an explicit guard would catch edge cases and fail fast:
🛡️ Suggested defensive check
Future<void> createOrder() async {
_createOrderStatus = Status.loading;
notifyListeners();
if (basketItems.isEmpty) {
_createOrderStatus = Status.failure('Your basket is empty');
notifyListeners();
return;
}
+ if (deliveryChoice == 'pickup' && selectedPickupPoint == null) {
+ _createOrderStatus = Status.failure('Please select a pickup point');
+ notifyListeners();
+ return;
+ }
+
final Map<String, dynamic> address = deliveryChoice == "pickup"🤖 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 585 - 594, The
deliveryDetails construction uses fallback 'unknown' values when deliveryChoice
== 'pickup' but selectedPickupPoint can be null; add a defensive guard before
building deliveryDetails: check deliveryChoice == 'pickup' and if
selectedPickupPoint == null then fail fast (throw an exception, return an error,
or log and abort submission) so you never send courier: 'unknown' or
pickupPointId: 'unknown'; update the function that builds/returns
deliveryDetails (referenced by deliveryChoice and selectedPickupPoint) to
perform this validation and handle the error path appropriately.
| if (inposts.isEmpty) { | ||
| return PickupPointsEmptyWidget(); | ||
| } | ||
| } else if (status.type == StatusType.success || status.type == StatusType.uninitialized) { |
There was a problem hiding this comment.
Treating uninitialized and success identically may cause confusing UX.
When status.type == StatusType.uninitialized, the UI renders the pickup content (courier list or points) before any fetch has occurred. This could display hardcoded mock data immediately, masking the fact that no real API call has been made.
Consider either:
- Showing a prompt to enter postcode first, or
- Only rendering content on
StatusType.success
🤖 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 298, The branch
treating StatusType.uninitialized the same as StatusType.success causes mock
pickup content to render before a fetch; update the conditional in the widget
that checks status.type so it only treats StatusType.success as the "show
results" path (i.e., replace the combined check in the clause that currently
reads "status.type == StatusType.success || status.type ==
StatusType.uninitialized" with a single check for StatusType.success), and add
an explicit branch for StatusType.uninitialized that renders a
prompt/placeholder asking the user to enter a postcode (or otherwise indicates
no data fetched yet) so the UI does not display pickup lists/points prematurely.
| ListTile( | ||
| title: Text('${courier.name} Pick-up Points'), | ||
| trailing: const Icon(Icons.arrow_forward_ios, size: 16), | ||
| onTap: () { | ||
| model.setSelectedCourier(courier); | ||
| model.fetchNearestPickupPoints(postcodeController.text.trim()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and confirm its existence
find . -name "delivery_options.dart" -type fRepository: Cherry-CIC/MVP
Length of output: 112
🏁 Script executed:
# Check the repository structure
ls -laRepository: Cherry-CIC/MVP
Length of output: 1739
🏁 Script executed:
# Look for model/view model files related to delivery or checkout
find . -path "*/checkout/*" -name "*.dart" -type f | head -20Repository: Cherry-CIC/MVP
Length of output: 1154
🏁 Script executed:
# Check the size of delivery_options.dart first
wc -l lib/features/checkout/widgets/delivery_options.dartRepository: Cherry-CIC/MVP
Length of output: 114
🏁 Script executed:
# Read the relevant section around lines 356-361
sed -n '350,370p' lib/features/checkout/widgets/delivery_options.dartRepository: Cherry-CIC/MVP
Length of output: 1265
🏁 Script executed:
# Check the checkout_view_model.dart to find the relevant methods and fields
wc -l lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 109
🏁 Script executed:
# Find fetchNearestPickupPoints method in the view model
grep -n "fetchNearestPickupPoints" lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 182
🏁 Script executed:
# Find nearestPickupPoints getter
grep -n "nearestPickupPoints" lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 265
🏁 Script executed:
# Find the _nearestInpost and _nearestPickupPoints fields
grep -n "_nearestInpost\|_nearestPickupPoints" lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 380
🏁 Script executed:
# Read the fetchNearestPickupPoints method implementation (lines around 374)
sed -n '374,410p' lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 1335
🏁 Script executed:
# Also check what's in _nearestPickupPoints initialization
sed -n '46,85p' lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 1114
🏁 Script executed:
# Check where nearestPickupPoints is used in the delivery_options.dart
grep -n "nearestPickupPoints" lib/features/checkout/widgets/delivery_options.dartRepository: Cherry-CIC/MVP
Length of output: 130
🏁 Script executed:
# Also check if nearestInpost is used anywhere in delivery_options.dart
grep -n "nearestInpost\|model.nearestInpost" lib/features/checkout/widgets/delivery_options.dartRepository: Cherry-CIC/MVP
Length of output: 40
🏁 Script executed:
# Check the rest of the nearestPickupPoints getter implementation
sed -n '84,92p' lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 423
🏁 Script executed:
# Let me check if _nearestInpost is used anywhere else in the codebase
rg "_nearestInpost" lib/features/checkout/Repository: Cherry-CIC/MVP
Length of output: 401
🏁 Script executed:
# Check if there's any conversion or transfer from _nearestInpost to _nearestPickupPoints
rg "nearestInpost\|_nearestPickupPoints" lib/features/checkout/checkout_view_model.dart -B 2 -A 2Repository: Cherry-CIC/MVP
Length of output: 40
🏁 Script executed:
# Check what PickupPointModel and InpostModel are
grep -n "class PickupPointModel\|class InpostModel" lib/features/checkout/ -rRepository: Cherry-CIC/MVP
Length of output: 40
🏁 Script executed:
# Find model files in the checkout feature
find lib/features/checkout -name "*.dart" | xargs grep -l "class PickupPointModel\|class InpostModel"Repository: Cherry-CIC/MVP
Length of output: 42
🏁 Script executed:
# Check the models directory
ls -la lib/features/checkout/models/Repository: Cherry-CIC/MVP
Length of output: 309
🏁 Script executed:
# Let me check what happens around line 373 in delivery_options.dart where nearestPickupPoints is used
sed -n '365,385p' lib/features/checkout/widgets/delivery_options.dartRepository: Cherry-CIC/MVP
Length of output: 1012
🏁 Script executed:
# Find where PickupPointModel and InpostModel are defined
find lib -name "*.dart" | xargs grep -l "class PickupPointModel"
find lib -name "*.dart" | xargs grep -l "class InpostModel"Repository: Cherry-CIC/MVP
Length of output: 132
🏁 Script executed:
# Check the showLocker usage to understand the state flow
grep -n "showLocker" lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 238
🏁 Script executed:
# Check if there's any logic that uses nearestInpost in the widget
grep -n "showLocker\|nearestInpost\|nearestPickupPoints" lib/features/checkout/widgets/delivery_options.dart | head -20Repository: Cherry-CIC/MVP
Length of output: 225
🏁 Script executed:
# Check the context around line 271 to understand the state flow
sed -n '265,290p' lib/features/checkout/widgets/delivery_options.dartRepository: Cherry-CIC/MVP
Length of output: 1191
🏁 Script executed:
# Read the core models to understand the structure
cat lib/core/models/pickup_point_model.dart
cat lib/core/models/inpost_model.dartRepository: Cherry-CIC/MVP
Length of output: 1368
🏁 Script executed:
# Let me verify that nearestInpost getter is not used anywhere in the UI
grep -r "nearestInpost" lib/features/checkout/widgets/Repository: Cherry-CIC/MVP
Length of output: 40
🏁 Script executed:
# Also check if _parseInpostList does any conversion to PickupPointModel
grep -n "_parseInpostList" lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 165
🏁 Script executed:
# Read the _parseInpostList method to understand the data transformation
sed -n '420,500p' lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 2706
🏁 Script executed:
# Let me check the _parseInpostList method to see what it does
sed -n '659,723p' lib/features/checkout/checkout_view_model.dartRepository: Cherry-CIC/MVP
Length of output: 1906
🏁 Script executed:
# Verify if there's any connection between nearestInpost and the UI by checking for any observers
grep -n "nearestInpost" lib/features/checkout/ -rRepository: Cherry-CIC/MVP
Length of output: 415
Fetch result is disconnected from UI display.
When a courier is tapped, fetchNearestPickupPoints is called and populates _nearestInpost with API results. However, the UI reads from the hardcoded _nearestPickupPoints list (filtered by courier ID), so the fetched data is never displayed. The showLocker flag is set based on API success, but the pickup points shown always come from mock data.
Two separate data models exist: InpostModel for the API response and PickupPointModel for the hardcoded UI list. The nearestInpost getter is not used in any widget.
This appears intentional for the development phase, but the data flow must be connected before production use.
🤖 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 356 - 361,
The UI is still bound to the hardcoded _nearestPickupPoints instead of the API
result stored in _nearestInpost, so tapping a courier updates
model._nearestInpost via fetchNearestPickupPoints but the widgets never read it;
update the data flow so the ListTile tap path (setSelectedCourier,
fetchNearestPickupPoints) results are displayed by wiring the UI to the
model.nearestInpost (or merging _nearestInpost into the existing
PickupPointModel list) and ensure showLocker is used to choose which list to
render; specifically change the widgets that currently read _nearestPickupPoints
to prefer model.nearestInpost when non-empty (or after successful fetch), and
keep setSelectedCourier, fetchNearestPickupPoints, nearestInpost,
_nearestInpost, _nearestPickupPoints and showLocker consistent so the fetched
points replace or augment the mock list.
|
No worries @CherryCIC , happy to help! |
|
We're just waiting on a second approval |
|
Will have a look tonight... |
JaseElder
left a comment
JaseElder
left a comment
There was a problem hiding this comment.
There are some unresolved coderabbit comments that need looked at here.
5 participants
Description
This PR implements the "Ship to pick-up point" feature for checkout delivery options, resolving all 5 scenarios outlined in the issue spec. It replaces the mock hard-coded InPost locker logic with a dynamic courier and pickup point selection flow.
Key Changes:
deliveryMethod,courier, andpickupPointIdare sent to the backend.Testing Performed
deliveryMethod: "pickup_point",courier, andpickupPointId.Screenshots of the 3 main flow steps are attached below.
Summary by CodeRabbit
Release Notes