feat(SCRUM-73): SCRUM-73-acc#9
Merged
Merged
Conversation
alibesar7
changed the base branch from
main
to
feature/SCRUM-87-drivers-orders-details
alibesar7
changed the title
alibesar7
changed the title
alibesar7
changed the base branch from
feature/SCRUM-87-drivers-orders-details
to
feature/SCRUM-80-home-order
Collaborator
🚧 Automated Code ReviewScore: 0/100 📋 Summary
🔍 Detected Issues
(...and 13 more issues) Review generated for Alibesar |
There was a problem hiding this comment.
Pull request overview
This pull request implements order acceptance functionality for drivers in a Flutter tracking application, including Cloud Firestore integration for real-time order and driver data synchronization, responsive UI updates, and localization improvements.
Changes:
- Added Cloud Firestore integration to upload driver location and order data when accepting orders
- Implemented responsive design across driver order UI widgets using MediaQuery-based sizing
- Migrated from string literals to LocaleKeys constants for type-safe translations
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pubspec.yaml | Added cloud_firestore dependency (version 6.1.2) |
| pubspec.lock | Updated lock file with cloud_firestore and related platform interface packages |
| windows/flutter/generated_plugins.cmake | Added cloud_firestore to Windows plugin list |
| windows/flutter/generated_plugin_registrant.cc | Registered cloud_firestore plugin for Windows |
| macos/Flutter/GeneratedPluginRegistrant.swift | Registered cloud_firestore plugin for macOS |
| web/firebase-messaging-sw.js | Added Firebase messaging service worker for web push notifications |
| lib/app/config/network/network_module.dart | Registered FirebaseFirestore instance in dependency injection |
| lib/app/config/di/di.config.dart | Generated DI configuration for new use cases and Firestore |
| lib/app/config/auth_storage/auth_storage.dart | Added methods to persist, retrieve, and clear order IDs |
| lib/app/core/api_manger/api_client.dart | Removed unused retrofit/dio import |
| lib/app/core/app_constants.dart | Added floweryRider constant |
| lib/features/home/api/driverOrderDataS_imp.dart | Implemented getProfile method in data source |
| lib/features/home/data/datascourse/driverOrderDatascource.dart | Added getProfile method signature |
| lib/features/home/data/repo/driverOrderRepo_impl.dart | Implemented getProfile in repository |
| lib/features/home/domain/repo/driverOrderRepo.dart | Added getProfile method to repository interface |
| lib/features/home/domain/usecase/upload_driver_fire_data_use_case.dart | New use case to upload driver data to Firestore |
| lib/features/home/domain/usecase/upload_order_fire_data_use_case.dart | New use case to upload order data to Firestore |
| lib/features/home/presentation/manger/driverorderIntent.dart | Added AcceptOrder intent |
| lib/features/home/presentation/manger/driverorderCubit.dart | Implemented _acceptOrder method with location permission handling and Firestore data upload |
| lib/features/home/presentation/pages/driverOrderScreen.dart | Added Scaffold with AppBar displaying "Flowery Rider" title |
| lib/features/home/presentation/widgets/driverScreenBody.dart | Updated error messages to use LocaleKeys and connected Accept button to AcceptOrder intent |
| lib/features/home/presentation/widgets/driverOrderItem.dart | Made UI responsive with MediaQuery sizing and migrated to LocaleKeys |
| lib/features/home/presentation/widgets/driverOrderSectionLabel.dart | Updated to use responsive font sizing |
| lib/features/home/presentation/widgets/driverOrderInfoCard.dart | Made all sizing responsive using MediaQuery |
| lib/features/home/presentation/widgets/driverOrderButton.dart | Updated padding and font size to be responsive |
| lib/generated/locale_keys.g.dart | Added new locale keys and reordered discount key |
| assets/translations/en.json | Added "floweryRider" translation |
| assets/translations/ar.json | Added "floweryRider" translation in Arabic |
| test/features/home/presentation/pages/driverOrderScreen_test.dart | Updated test to mock new use cases |
| test/features/home/presentation/manger/driverorderCubit_test.dart | Updated test to mock new use cases |
| test/features/home/presentation/widgets/driverOrderSectionLabel_test.dart | Deleted test file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+63
to
+100
| final token = await _authStorage.getToken(); | ||
| if (token == null) return; | ||
|
|
||
| final result = await _driverOrderRepository.getProfile(token); | ||
|
|
||
| if (result is SuccessApiResult) { | ||
| final profile = (result as SuccessApiResult).data; | ||
| if (profile.driver != null) { | ||
| try { | ||
| final position = await _determinePosition(); | ||
| if (position == null) { | ||
| if (kDebugMode) | ||
| print("Location permission denied or service disabled."); | ||
| return; | ||
| } | ||
|
|
||
| final deviceToken = await FirebaseMessaging.instance.getToken(); | ||
| await _uploadDriverFireDataUseCase( | ||
| profile.driver!, | ||
| lat: position.latitude, | ||
| lng: position.longitude, | ||
| deviceToken: deviceToken, | ||
| ); | ||
|
|
||
| await _uploadOrderFireDataUseCase( | ||
| order: order, | ||
| driverId: profile.driver?.Id ?? '', | ||
| ); | ||
|
|
||
| if (order.id != null) { | ||
| await _authStorage.saveOrderId(order.id!); | ||
| } | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print("Firestore/Location Error: $e"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling: The _acceptOrder method does not update the UI state or provide any user feedback when location permissions are denied, location services are disabled, or when Firestore operations fail. Users will experience silent failures with no indication of what went wrong.
Suggested change
| final token = await _authStorage.getToken(); | |
| if (token == null) return; | |
| final result = await _driverOrderRepository.getProfile(token); | |
| if (result is SuccessApiResult) { | |
| final profile = (result as SuccessApiResult).data; | |
| if (profile.driver != null) { | |
| try { | |
| final position = await _determinePosition(); | |
| if (position == null) { | |
| if (kDebugMode) | |
| print("Location permission denied or service disabled."); | |
| return; | |
| } | |
| final deviceToken = await FirebaseMessaging.instance.getToken(); | |
| await _uploadDriverFireDataUseCase( | |
| profile.driver!, | |
| lat: position.latitude, | |
| lng: position.longitude, | |
| deviceToken: deviceToken, | |
| ); | |
| await _uploadOrderFireDataUseCase( | |
| order: order, | |
| driverId: profile.driver?.Id ?? '', | |
| ); | |
| if (order.id != null) { | |
| await _authStorage.saveOrderId(order.id!); | |
| } | |
| } catch (e) { | |
| if (kDebugMode) { | |
| print("Firestore/Location Error: $e"); | |
| } | |
| } | |
| } | |
| // Indicate that an accept operation is in progress. | |
| emit(state.copyWith(orderResource: Resource.loading())); | |
| final token = await _authStorage.getToken(); | |
| if (token == null) { | |
| emit( | |
| state.copyWith( | |
| orderResource: Resource.error("User not authenticated"), | |
| ), | |
| ); | |
| return; | |
| } | |
| final result = await _driverOrderRepository.getProfile(token); | |
| if (result is SuccessApiResult) { | |
| final profile = (result as SuccessApiResult).data; | |
| if (profile.driver == null) { | |
| emit( | |
| state.copyWith( | |
| orderResource: Resource.error("Driver profile not found"), | |
| ), | |
| ); | |
| return; | |
| } | |
| try { | |
| final position = await _determinePosition(); | |
| if (position == null) { | |
| if (kDebugMode) { | |
| print("Location permission denied or service disabled."); | |
| } | |
| emit( | |
| state.copyWith( | |
| orderResource: Resource.error( | |
| "Location permission denied or service disabled", | |
| ), | |
| ), | |
| ); | |
| return; | |
| } | |
| final deviceToken = await FirebaseMessaging.instance.getToken(); | |
| await _uploadDriverFireDataUseCase( | |
| profile.driver!, | |
| lat: position.latitude, | |
| lng: position.longitude, | |
| deviceToken: deviceToken, | |
| ); | |
| await _uploadOrderFireDataUseCase( | |
| order: order, | |
| driverId: profile.driver?.Id ?? '', | |
| ); | |
| if (order.id != null) { | |
| await _authStorage.saveOrderId(order.id!); | |
| } | |
| } catch (e) { | |
| if (kDebugMode) { | |
| print("Firestore/Location Error: $e"); | |
| } | |
| emit( | |
| state.copyWith( | |
| orderResource: Resource.error("Failed to accept order"), | |
| ), | |
| ); | |
| } | |
| } else if (result is ErrorApiResult) { | |
| final error = (result as ErrorApiResult).error; | |
| emit( | |
| state.copyWith( | |
| orderResource: Resource.error(error), | |
| ), | |
| ); |
| 'adress': | ||
| '${order.shippingAddress?.street ?? ''}, ${order.shippingAddress?.city ?? ''}', | ||
| 'name': '${order.user?.firstName ?? ''} ${order.user?.lastName ?? ''}', | ||
| 'user_id': order.user?.id ?? '', |
There was a problem hiding this comment.
Data duplication: The 'user_id' field appears twice - once at the root level (line 46) and once inside the 'userAddress' object (line 44). This redundancy could lead to data inconsistencies and should be consolidated to a single location.
Suggested change
| 'user_id': order.user?.id ?? '', |
| import 'package:tracking_app/features/home/data/model/response/orderRespons.dart'; | ||
| import 'package:tracking_app/features/home/domain/repo/driverOrderRepo.dart'; | ||
| import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart'; | ||
| import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart'; |
There was a problem hiding this comment.
Duplicate import detected. The import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart' is already present on line 10.
Suggested change
| import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart'; |
| '${order.shippingAddress?.street ?? ''}, ${order.shippingAddress?.city ?? ''}', | ||
| }, | ||
| 'userAddress': { | ||
| 'adress': |
There was a problem hiding this comment.
Typo in field name 'adress' should be 'address'. This inconsistency exists in the Firestore document structure while line 42 correctly uses 'address' in the concatenation string.
Suggested change
| 'adress': | |
| 'address': |
Comment on lines
+62
to
+102
| Future<void> _acceptOrder(Order order) async { | ||
| final token = await _authStorage.getToken(); | ||
| if (token == null) return; | ||
|
|
||
| final result = await _driverOrderRepository.getProfile(token); | ||
|
|
||
| if (result is SuccessApiResult) { | ||
| final profile = (result as SuccessApiResult).data; | ||
| if (profile.driver != null) { | ||
| try { | ||
| final position = await _determinePosition(); | ||
| if (position == null) { | ||
| if (kDebugMode) | ||
| print("Location permission denied or service disabled."); | ||
| return; | ||
| } | ||
|
|
||
| final deviceToken = await FirebaseMessaging.instance.getToken(); | ||
| await _uploadDriverFireDataUseCase( | ||
| profile.driver!, | ||
| lat: position.latitude, | ||
| lng: position.longitude, | ||
| deviceToken: deviceToken, | ||
| ); | ||
|
|
||
| await _uploadOrderFireDataUseCase( | ||
| order: order, | ||
| driverId: profile.driver?.Id ?? '', | ||
| ); | ||
|
|
||
| if (order.id != null) { | ||
| await _authStorage.saveOrderId(order.id!); | ||
| } | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print("Firestore/Location Error: $e"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing state update after accepting order: After successfully accepting an order, the method should either remove the order from the list or refresh the pending orders list. Currently, the accepted order remains visible in the UI until manual refresh.
| firebase_messaging: ^16.1.1 | ||
| flutter_local_notifications: ^20.0.0 | ||
| firebase_crashlytics: ^5.0.7 | ||
| cloud_firestore: 6.1.2 |
There was a problem hiding this comment.
Inconsistent version format: The cloud_firestore dependency uses version 6.1.2 without a caret (^) prefix, while all other dependencies in the file use the caret prefix (e.g., firebase_messaging: ^16.1.1). This should be changed to cloud_firestore: ^6.1.2 for consistency with the rest of the project.
Suggested change
| cloud_firestore: 6.1.2 | |
| cloud_firestore: ^6.1.2 |
Comment on lines
+1
to
+2
| importScripts("https://www.gstatic.com/firebasejs/8.10.0/firebase-app.js"); | ||
| importScripts("https://www.gstatic.com/firebasejs/8.10.0/firebase-messaging.js"); |
There was a problem hiding this comment.
Outdated Firebase SDK version: The service worker uses Firebase JavaScript SDK version 8.10.0, which is outdated. Consider upgrading to Firebase v9+ modular SDK for better performance, smaller bundle sizes, and improved tree-shaking. The current v8 SDK is in maintenance mode.
Suggested change
| importScripts("https://www.gstatic.com/firebasejs/8.10.0/firebase-app.js"); | |
| importScripts("https://www.gstatic.com/firebasejs/8.10.0/firebase-messaging.js"); | |
| importScripts("https://www.gstatic.com/firebasejs/9.6.11/firebase-app-compat.js"); | |
| importScripts("https://www.gstatic.com/firebasejs/9.6.11/firebase-messaging-compat.js"); |
Comment on lines
+37
to
+45
| 'userAddress': | ||
| '${order.shippingAddress?.street ?? ''}, ${order.shippingAddress?.city ?? ''}', | ||
| }, | ||
| 'userAddress': { | ||
| 'adress': | ||
| '${order.shippingAddress?.street ?? ''}, ${order.shippingAddress?.city ?? ''}', | ||
| 'name': '${order.user?.firstName ?? ''} ${order.user?.lastName ?? ''}', | ||
| 'user_id': order.user?.id ?? '', | ||
| }, |
There was a problem hiding this comment.
Data duplication: The 'userAddress' field appears twice in the document structure - once inside 'oder_dt' (line 37-38) and once at the root level (line 40-45). This creates redundant data that could lead to inconsistencies if one is updated without the other. Consider consolidating this data or clearly documenting why both are needed.
| import 'package:tracking_app/features/home/data/model/response/orderRespons.dart'; | ||
| import 'package:tracking_app/features/home/domain/repo/driverOrderRepo.dart'; | ||
| import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart'; | ||
| import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart'; |
There was a problem hiding this comment.
Duplicate import detected. The import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart' is already present on line 12.
Suggested change
| import 'package:tracking_app/features/home/domain/usecase/getdriverOrderUsecase.dart'; |
Comment on lines
+3
to
+11
|
|
||
| firebase.initializeApp({ | ||
| apiKey: "AIzaSyDKWdkFjeKkEAfKFrMO2svs48t2d9OqRGw", | ||
| appId: "1:725835190067:web:86225b1572d53a90e53846", | ||
| messagingSenderId: "725835190067", | ||
| projectId: "elevate-flower-app", | ||
| authDomain: "elevate-flower-app.firebaseapp.com", | ||
| storageBucket: "elevate-flower-app.firebasestorage.app" | ||
| }); |
There was a problem hiding this comment.
Security vulnerability: Firebase API keys and project configuration are hardcoded in a publicly accessible service worker file. These credentials should be loaded from environment variables or a secure configuration mechanism to prevent unauthorized access to your Firebase project.
Suggested change
| firebase.initializeApp({ | |
| apiKey: "AIzaSyDKWdkFjeKkEAfKFrMO2svs48t2d9OqRGw", | |
| appId: "1:725835190067:web:86225b1572d53a90e53846", | |
| messagingSenderId: "725835190067", | |
| projectId: "elevate-flower-app", | |
| authDomain: "elevate-flower-app.firebaseapp.com", | |
| storageBucket: "elevate-flower-app.firebasestorage.app" | |
| }); | |
| importScripts("/firebase-config.js"); | |
| firebase.initializeApp(self.firebaseConfig); |
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
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.
No description provided.