Feat/orders details screen #70
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the order management system within the application. Key changes include the addition of local and remote data sources for orders, an order repository, and the corresponding models and use cases. The Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
closes #71 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (18)
lib/features/orders/presentation/bloc/order_bloc.dart (3)
36-36: Provide user-friendly error messages inOrdersErrorstateWhen emitting the
OrdersErrorstate, consider providing a user-friendly error message instead of the raw failure message.Modify the emitted error message to be more user-friendly.
63-63: Provide user-friendly error messages inOrdersErrorstateWhen emitting the
OrdersErrorstate after a failed order update, consider providing a user-friendly error message.Modify the emitted error message to be more informative for the user.
67-67: Ensure state consistency after order updateAfter successfully updating an order, the bloc adds a
LoadOrdersevent. This could cause unnecessary reloading. Consider updating the local state directly to reflect the change without reloading all orders.Refactor the state management to update the specific order in the current state.
lib/features/orders/data/repositories/order_repository_impl.dart (1)
66-66: Avoid usingnullinRightfor void return typesReturning
Right(null)may be confusing. Instead, useRight(unit)from thedartzpackage to represent a successful void result.Apply this diff:
- return const Right<Failure, void>(null); + return Right<Failure, void>(unit);lib/features/orders/presentation/widgets/order_item.dart (3)
58-64: Ensure color contrast for accessibilityThe color used in the status label background and text should have sufficient contrast to be readable by users with visual impairments.
Consider reviewing the color choices to ensure accessibility compliance.
85-97: Consider default case handling in_getStatusColorWhile all
OrderStatusenum values are currently handled, adding a default case can prevent potential issues if new statuses are added in the future.Apply this diff to include a default case:
switch (status) { // existing cases + default: + return Colors.grey; }
41-45: Truncate long business names to prevent overflowEnsure that very long business names do not cause layout issues by adding ellipses or limiting the maximum lines.
The current code uses
maxLines: 1andoverflow: TextOverflow.ellipsis, which is good.lib/features/orders/data/datasources/order_local_data_source.dart (1)
63-64: Consider wrapping deletions in transactions for data integrityWhile you're already using a transaction, ensure that deletions are properly handled to prevent partial data states.
Review the transaction handling to ensure atomicity.
lib/features/orders/domain/repositories/order_repository.dart (1)
6-12: Consider adding documentation comments.Adding documentation comments (///) for the abstract class and its methods would improve code maintainability and IDE support.
+/// Repository interface for managing orders. abstract class OrderRepository { + /// Retrieves a list of orders. + /// Returns a [Future] that completes with either a [Failure] or a [List] of [Order]. Future<Either<Failure, List<entities.Order>>> getOrders(); + /// Updates the status of an order. + /// + /// Parameters: + /// - [orderId]: The unique identifier of the order + /// - [newStatus]: The new status to be applied + /// + /// Returns a [Future] that completes with either a [Failure] or void. Future<Either<Failure, void>> updateOrderStatus( String orderId, entities.OrderStatus newStatus, ); }lib/features/orders/domain/usecases/get_orders.dart (1)
7-15: Consider using NoParams for consistency with clean architecture.The implementation looks good, but for consistency with clean architecture patterns, consider using a NoParams class for the call method.
class GetOrders { const GetOrders(this.repository); final OrderRepository repository; - Future<Either<Failure, List<order_entity.Order>>> call() async { + Future<Either<Failure, List<order_entity.Order>>> call(NoParams params) async { return repository.getOrders(); } } + +/// Represents no parameters for use cases that don't require input +class NoParams extends Equatable { + @override + List<Object?> get props => []; +}lib/core/utils/date_utils.dart (1)
20-21: Consider adding documentation and examples.The new method looks good, but would benefit from documentation similar to other methods in the class.
+ /// Formats the given date and time into a string in the format "d MMM y, HH:mm". + /// Example output: "27 Apr 2023, 14:30" static String formatDateTime(DateTime dateTime) { return DateFormat('d MMM y, HH:mm').format(dateTime); }lib/features/orders/presentation/bloc/order_state.dart (1)
24-31: Consider adding @immutable annotation.The OrdersError state implementation looks good, but could benefit from explicit immutability annotation.
+import 'package:meta/meta.dart'; +@immutable class OrdersError extends OrderState { const OrdersError(this.message);lib/features/orders/domain/usecases/update_order_status.dart (1)
17-28: Consider adding toString override for debugging.The params class implementation looks good but could benefit from a toString override for better debugging experience.
@override List<Object?> get props => <Object?>[orderId, newStatus]; + + @override + String toString() => 'UpdateOrderStatusParams(orderId: $orderId, newStatus: $newStatus)';lib/features/orders/domain/entities/order.dart (1)
3-24: Add documentation and serialization methods to OrderStatus enumConsider adding:
- Documentation comments explaining each status and valid state transitions
- A
fromStringfactory constructor for safe deserialization/// Represents the possible states of an order in the system. enum OrderStatus { /// Initial state when order is created pending, /// Order has been confirmed by business confirmed, /// Order is ready for pickup/delivery ready, /// Order has been delivered/picked up completed, /// Order has been cancelled cancelled; /// Creates an OrderStatus from a string representation static OrderStatus fromString(String status) { return OrderStatus.values.firstWhere( (s) => s.name == status, orElse: () => throw ArgumentError('Invalid status: $status'), ); } String get displayName => name[0].toUpperCase() + name.substring(1); }lib/features/orders/presentation/screens/order_list_screen.dart (1)
26-26: Consider adding pull-to-refresh functionalityThe LoadOrders event is only dispatched once when the screen builds. Consider adding pull-to-refresh for better UX.
RefreshIndicator( onRefresh: () async { context.read<OrderBloc>().add(LoadOrders()); }, child: ListView.builder(...), )lib/features/orders/data/models/order_model.dart (1)
56-81: Add copyWith method to OrderItemModelFor better immutability support, consider adding a copyWith method.
OrderItemModel copyWith({ String? id, String? name, int? quantity, double? price, }) { return OrderItemModel( id: id ?? this.id, name: name ?? this.name, quantity: quantity ?? this.quantity, price: price ?? this.price, ); }lib/features/orders/presentation/screens/order_details_screen.dart (2)
35-51: Consider improving status indicator accessibility.The status indicator's color opacity of 0.1 might not provide sufficient contrast for all users. Consider:
- Increasing the background opacity
- Adding a semantic label for screen readers
Container( padding: const EdgeInsets.symmetric( horizontal: 12, vertical: 6, ), decoration: BoxDecoration( - color: _getStatusColor(order.status).withOpacity(0.1), + color: _getStatusColor(order.status).withOpacity(0.15), borderRadius: BorderRadius.circular(16), ), child: Text( order.status.displayName, + semanticsLabel: 'Order status: ${order.status.displayName}', style: theme.textTheme.titleMedium?.copyWith( color: _getStatusColor(order.status), fontWeight: FontWeight.w500, ), ), ),
112-125: Consider future-proofing status colors and theme integration.The current implementation could be improved by:
- Adding a default case for future status values
- Using theme colors instead of hardcoded ones
Color _getStatusColor(order_entity.OrderStatus status) { switch (status) { case order_entity.OrderStatus.pending: - return Colors.orange; + return Theme.of(context).colorScheme.warning ?? Colors.orange; case order_entity.OrderStatus.confirmed: - return Colors.blue; + return Theme.of(context).colorScheme.primary; case order_entity.OrderStatus.ready: - return Colors.green; + return Theme.of(context).colorScheme.success ?? Colors.green; case order_entity.OrderStatus.completed: - return Colors.purple; + return Theme.of(context).colorScheme.tertiary ?? Colors.purple; case order_entity.OrderStatus.cancelled: - return Colors.red; + return Theme.of(context).colorScheme.error; + default: + return Theme.of(context).colorScheme.primary; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
lib/core/utils/date_utils.dart(1 hunks)lib/features/orders/data/datasources/order_local_data_source.dart(1 hunks)lib/features/orders/data/datasources/order_remote_data_source.dart(1 hunks)lib/features/orders/data/models/order_model.dart(1 hunks)lib/features/orders/data/repositories/order_repository_impl.dart(1 hunks)lib/features/orders/domain/entities/order.dart(1 hunks)lib/features/orders/domain/repositories/order_repository.dart(1 hunks)lib/features/orders/domain/usecases/get_orders.dart(1 hunks)lib/features/orders/domain/usecases/update_order_status.dart(1 hunks)lib/features/orders/presentation/bloc/order_bloc.dart(1 hunks)lib/features/orders/presentation/bloc/order_event.dart(1 hunks)lib/features/orders/presentation/bloc/order_state.dart(1 hunks)lib/features/orders/presentation/screens/order_details_screen.dart(1 hunks)lib/features/orders/presentation/screens/order_list_screen.dart(3 hunks)lib/features/orders/presentation/widgets/order_item.dart(1 hunks)lib/injection_container.dart(2 hunks)
🔇 Additional comments (11)
lib/features/orders/data/datasources/order_local_data_source.dart (2)
167-169: Handle date-time conversion carefully
When updating completedAt, ensure that the date-time is stored in a consistent format and time zone.
Confirm that DateTime.now().toIso8601String() stores the date in the correct format. If necessary, consider using UTC.
77-79: Ensure completedAt is always stored correctly
When inserting into the database, make sure that completedAt is handled consistently, even when it's null.
Run the following script to ensure that completedAt field is stored correctly:
✅ Verification successful
completedAt field is handled correctly in the codebase
The code handles the completedAt field appropriately:
- In the database schema, it's defined as nullable (
TEXT) - When inserting orders, it's only included if the value is non-null
- When updating order status to completed, it's properly set:
- In remote datasource: uses server timestamp
- In local datasource: uses current timestamp
- When parsing orders, it correctly handles both null and non-null cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all orders have 'completedAt' set appropriately.
# List orders without 'completedAt' where status is 'completed'
sqlite3 orders.db "SELECT id FROM orders WHERE status = 'completed' AND completedAt IS NULL;"
Length of output: 199
Script:
#!/bin/bash
# Let's analyze how completedAt is handled in the codebase
# Find all references to completedAt field
rg "completedAt" -A 3 -B 3
# Also check for any database schema definitions
fd "schema|model" --type f
Length of output: 7322
lib/features/orders/domain/repositories/order_repository.dart (1)
6-12: LGTM! Well-structured repository interface.
The repository pattern is well implemented with proper error handling using Either type.
lib/features/orders/presentation/bloc/order_event.dart (1)
13-24: LGTM! Well-structured event class.
The UpdateOrder event is well-implemented with proper immutability, required parameters, and correct Equatable implementation.
lib/core/utils/date_utils.dart (1)
17-18: LGTM! More readable date format.
The change from 'MM/dd' to 'd MMM' improves readability by using abbreviated month names instead of numbers.
lib/features/orders/presentation/bloc/order_state.dart (2)
5-6: LGTM! Good use of const constructor.
Adding const constructor to the base class enables compile-time constants for all state classes.
16-21: LGTM! Proper immutable state implementation.
Good implementation of the OrdersLoaded state with:
- Const constructor
- Final field
- Proper props override
lib/features/orders/domain/usecases/update_order_status.dart (2)
7-15: LGTM! Clean implementation of the use case.
Good implementation following clean architecture principles:
- Proper dependency injection
- Error handling with Either
- Single responsibility
12-14: Verify error handling in the repository implementation.
The use case correctly delegates to the repository, but let's verify the error handling implementation.
✅ Verification successful
Error handling in the repository implementation is properly implemented
The repository implementation has comprehensive error handling that covers:
- Network connectivity checks via
networkInfo.isConnected - Authentication errors through
AuthExceptionhandling - Network failures with appropriate
NetworkFailureresponses - Both remote and local data source updates with proper error propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the repository implementation to verify error handling
ast-grep --pattern 'class $_ implements OrderRepository {
$$$
Future<Either<Failure, void>> updateOrderStatus($_, $_) {
$$$
}
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Let's try a broader search for OrderRepository implementations
rg "implements OrderRepository" -A 10
# Also search for the updateOrderStatus method
rg "updateOrderStatus.*Failure.*void"
Length of output: 1116
Script:
#!/bin/bash
# Let's look at the complete implementation file to see the updateOrderStatus method
cat lib/features/orders/data/repositories/order_repository_impl.dart
Length of output: 2851
lib/features/orders/presentation/screens/order_details_screen.dart (1)
1-14: LGTM! Clean class structure and imports.
The class follows Flutter best practices with proper const constructor and required parameter.
lib/injection_container.dart (1)
30-35: LGTM! Well-organized imports for order feature.
All necessary components for the order feature are properly imported.
| Logger().e('Error fetching orders: $e'); | ||
| throw AuthException('Failed to fetch orders: $e'); | ||
| } |
There was a problem hiding this comment.
Avoid exposing sensitive error details in exceptions
Including the original exception message $e in the thrown AuthException might expose sensitive information. It's better to provide a generic error message to prevent potential information leakage.
Apply this diff to modify the exception message:
- throw AuthException('Failed to fetch orders: $e');
+ throw AuthException('Failed to fetch orders');📝 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.
| Logger().e('Error fetching orders: $e'); | |
| throw AuthException('Failed to fetch orders: $e'); | |
| } | |
| Logger().e('Error fetching orders: $e'); | |
| throw AuthException('Failed to fetch orders'); | |
| } |
| ) | ||
| .toList(); | ||
| } catch (e) { | ||
| Logger().e('Error fetching orders: $e'); |
There was a problem hiding this comment.
Avoid logging sensitive error information
Logging the exception $e can reveal sensitive details in the logs. Consider logging only the error type or a generic message.
Apply this diff to adjust the logging:
- Logger().e('Error fetching orders: $e');
+ Logger().e('Error fetching orders');📝 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.
| Logger().e('Error fetching orders: $e'); | |
| Logger().e('Error fetching orders'); |
| Logger().e('Error updating order status: $e'); | ||
| throw AuthException('Failed to update order status: $e'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent error handling and prevent sensitive data exposure
Similar to the getOrders method, include error handling in updateOrderStatus and avoid exposing sensitive error details in both logging and exceptions.
Apply this diff to add error handling and prevent information leakage:
} catch (e) {
- Logger().e('Error updating order status: $e');
- throw AuthException('Failed to update order status: $e');
+ Logger().e('Error updating order status');
+ throw AuthException('Failed to update order status');
}📝 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.
| Logger().e('Error updating order status: $e'); | |
| throw AuthException('Failed to update order status: $e'); | |
| } | |
| Logger().e('Error updating order status'); | |
| throw AuthException('Failed to update order status'); | |
| } |
| Future<void> createOrder(OrderModel order) async { | ||
| await _firestore.collection('orders').doc().set(order.toMap()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to createOrder method
The createOrder method lacks error handling, which can lead to unhandled exceptions. Wrap the method's body in a try-catch block to handle potential errors and maintain consistency with other methods.
Apply this diff to include error handling:
@override
Future<void> createOrder(OrderModel order) async {
+ try {
await _firestore.collection('orders').doc().set(order.toMap());
+ } catch (e) {
+ Logger().e('Error creating order');
+ throw AuthException('Failed to create order');
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| Logger().e('Failed to update order: ${failure.message}'); | ||
| emit(OrdersError(failure.message)); |
There was a problem hiding this comment.
Avoid logging sensitive error details
As with the _onLoadOrders method, avoid logging the failure message directly.
Apply this diff to adjust the logging:
- Logger().e('Failed to update order: ${failure.message}');
+ Logger().e('Failed to update order');📝 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.
| Logger().e('Failed to update order: ${failure.message}'); | |
| emit(OrdersError(failure.message)); | |
| Logger().e('Failed to update order'); | |
| emit(OrdersError(failure.message)); |
| factory OrderModel.fromMap(Map<String, dynamic> map, String id) { | ||
| return OrderModel( | ||
| id: id, | ||
| businessId: map['businessId'] as String, | ||
| businessName: map['businessName'] as String, | ||
| items: (map['items'] as List<dynamic>) | ||
| .map( | ||
| (dynamic item) => | ||
| OrderItemModel.fromMap(item as Map<String, dynamic>), | ||
| ) | ||
| .toList(), | ||
| totalAmount: (map['totalAmount'] as num).toDouble(), | ||
| status: order_entity.OrderStatus.values.firstWhere( | ||
| (order_entity.OrderStatus s) => s.name == (map['status'] as String), | ||
| ), | ||
| createdAt: (map['createdAt'] as Timestamp).toDate(), | ||
| completedAt: map['completedAt'] != null | ||
| ? (map['completedAt'] as Timestamp).toDate() | ||
| : null, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling to fromMap factory
The fromMap factory should handle potential null or invalid values from Firestore.
factory OrderModel.fromMap(Map<String, dynamic> map, String id) {
try {
return OrderModel(
id: id,
businessId: map['businessId'] as String? ??
throw FormatException('businessId is required'),
// ... similar null checks for other fields
);
} catch (e) {
throw FormatException('Failed to parse OrderModel: $e');
}
}| Map<String, dynamic> toMap() { | ||
| return <String, dynamic>{ | ||
| 'businessId': businessId, | ||
| 'businessName': businessName, | ||
| 'items': items | ||
| .map( | ||
| (order_entity.OrderItem item) => (item as OrderItemModel).toMap(), | ||
| ) | ||
| .toList(), | ||
| 'totalAmount': totalAmount, | ||
| 'status': status.name, | ||
| 'createdAt': Timestamp.fromDate(createdAt), | ||
| if (completedAt != null) 'completedAt': Timestamp.fromDate(completedAt!), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add type safety to toMap serialization
The type casting in the items map operation could fail at runtime.
Map<String, dynamic> toMap() {
if (items.any((item) => item is! OrderItemModel)) {
throw StateError(
'All items must be OrderItemModel instances for serialization',
);
}
return <String, dynamic>{
'businessId': businessId,
// ... rest of the implementation
};
}| ...order.items.map( | ||
| (order_entity.OrderItem item) => Padding( | ||
| padding: const EdgeInsets.only(bottom: 12.0), | ||
| child: Row( | ||
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| children: <Widget>[ | ||
| Expanded( | ||
| child: Text( | ||
| '${item.quantity}x ${item.name}', | ||
| style: theme.textTheme.bodyLarge, | ||
| ), | ||
| ), | ||
| Text( | ||
| 'COP ${item.price.toStringAsFixed(2)}', | ||
| style: theme.textTheme.bodyLarge, | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve currency formatting and text handling.
Consider these improvements:
- Use proper currency formatting instead of hardcoded 'COP'
- Handle potential text overflow for long item names
...order.items.map(
(order_entity.OrderItem item) => Padding(
padding: const EdgeInsets.only(bottom: 12.0),
child: Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: <Widget>[
Expanded(
child: Text(
'${item.quantity}x ${item.name}',
+ overflow: TextOverflow.ellipsis,
style: theme.textTheme.bodyLarge,
),
),
Text(
- 'COP ${item.price.toStringAsFixed(2)}',
+ NumberFormat.currency(
+ symbol: 'COP ',
+ decimalDigits: 2,
+ ).format(item.price),
style: theme.textTheme.bodyLarge,
),
],
),
),
),Don't forget to add the import:
import 'package:intl/intl.dart';| // Total | ||
| Row( | ||
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| children: <Widget>[ | ||
| Text( | ||
| 'Total', | ||
| style: theme.textTheme.titleLarge, | ||
| ), | ||
| Text( | ||
| 'COP ${order.totalAmount.toStringAsFixed(2)}', | ||
| style: theme.textTheme.titleLarge?.copyWith( | ||
| fontWeight: FontWeight.bold, | ||
| ), | ||
| ), | ||
| ], | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent currency formatting for total amount.
Apply the same currency formatting improvement as suggested for the order items.
Text(
- 'COP ${order.totalAmount.toStringAsFixed(2)}',
+ NumberFormat.currency(
+ symbol: 'COP ',
+ decimalDigits: 2,
+ ).format(order.totalAmount),
style: theme.textTheme.titleLarge?.copyWith(
fontWeight: FontWeight.bold,
),
),Committable suggestion skipped: line range outside the PR's diff.
| serviceLocator.registerFactory( | ||
| () => OrderBloc( | ||
| getOrders: serviceLocator(), | ||
| updateOrderStatus: serviceLocator(), | ||
| ), | ||
| ); | ||
|
|
||
| // Use cases | ||
| serviceLocator.registerLazySingleton(() => GetOrders(serviceLocator())); | ||
| serviceLocator | ||
| .registerLazySingleton(() => UpdateOrderStatus(serviceLocator())); | ||
|
|
||
| // Repository | ||
| serviceLocator.registerLazySingleton<OrderRepository>( | ||
| () => OrderRepositoryImpl( | ||
| remoteDataSource: serviceLocator(), | ||
| localDataSource: serviceLocator(), | ||
| networkInfo: serviceLocator(), | ||
| ), | ||
| ); | ||
|
|
||
| // Data sources | ||
| serviceLocator.registerLazySingleton<OrderRemoteDataSource>( | ||
| () => OrderRemoteDataSourceImpl( | ||
| firestore: serviceLocator(), | ||
| auth: serviceLocator(), | ||
| ), | ||
| ); | ||
| serviceLocator.registerLazySingleton<OrderLocalDataSource>( | ||
| () => OrderLocalDataSourceImpl(), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding network error handling in OrderBloc.
While the dependency injection setup is good, consider injecting InternetConnectionBloc for proper network error handling, similar to ProfileBloc implementation.
serviceLocator.registerFactory(
() => OrderBloc(
getOrders: serviceLocator(),
updateOrderStatus: serviceLocator(),
+ internetConnectionBloc: serviceLocator(),
),
);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation