Closed totzk9 closed 4 months ago
β±οΈ Estimated effort to review: 2 π΅π΅βͺβͺβͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Key issues to review Data Type Consistency Ensure that the data types for `emailVerified` and `phoneNumberVerified` are consistently handled as booleans across all methods (`fromJson`, `toJson`, `toString`). Currently, there's a potential issue in `fromJson` where `phoneNumberVerified` could throw if not explicitly a boolean. Type Safety The change from `dynamic` to `Object` in the equality operator might introduce type safety issues if not handled properly elsewhere in the code. Ensure that this change is compatible with all uses of the `AuthNotifier` class. |
Category | Suggestion | Score |
Possible bug |
β Add null checks to prevent runtime errors when parsing new fields from JSON___Suggestion Impact:The commit added a null check for the phoneNumber field, which aligns with the suggestion to add null checks for the new fields. code diff: ```diff - phoneNumber: json['phoneNumber'] as String, + phoneNumber: json['phoneNumber'] ?? '', ```emailVerified , phoneNumber , and phoneNumberVerified when parsing them from JSON. This will prevent potential runtime errors if these fields are missing or null in the JSON data.** [packages/nhost_sdk/lib/src/api/auth_api_types.dart [139-141]](https://github.com/nhost/nhost-dart/pull/140/files#diff-a611d97d65069a8f508b41d5dced2816a657086b86cfa99be35e282a79cd7a62R139-R141) ```diff -emailVerified: json['emailVerified'] as bool, -phoneNumber: json['phoneNumber'] as String, -phoneNumberVerified: json['phoneNumberVerified'] as bool, +emailVerified: json['emailVerified'] != null ? json['emailVerified'] as bool : false, +phoneNumber: json['phoneNumber'] != null ? json['phoneNumber'] as String : '', +phoneNumberVerified: json['phoneNumberVerified'] != null ? json['phoneNumberVerified'] as bool : false, ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Adding null checks is crucial to prevent potential runtime errors when the JSON data does not contain the new fields. This improves the robustness of the code. | 9 |
Best practice |
Implement checks to ensure
___
**To ensure data consistency, consider implementing checks or constraints on the | 8 |
Enhancement |
Validate the
___
**Ensure that the | 7 |
Maintainability |
Refactor boolean to string conversion into a reusable method___ **For better maintainability and to avoid redundancy, consider creating a method thathandles the conversion of the new boolean fields emailVerified and phoneNumberVerified to string. This method can be reused in different parts of the class.** [packages/nhost_sdk/lib/src/api/auth_api_types.dart [157-159]](https://github.com/nhost/nhost-dart/pull/140/files#diff-a611d97d65069a8f508b41d5dced2816a657086b86cfa99be35e282a79cd7a62R157-R159) ```diff -'emailVerified': emailVerified, -'phoneNumberVerified': phoneNumberVerified, +'emailVerified': convertBoolToString(emailVerified), +'phoneNumberVerified': convertBoolToString(phoneNumberVerified), +// Method to be added in the class: +String convertBoolToString(bool value) => value.toString(); + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Creating a reusable method for boolean to string conversion can improve maintainability, but it is a minor enhancement and not critical for functionality. | 6 |
Based on https://github.com/nhost/hasura-auth/blob/main/go/api/openapi.yaml#L504-L569 it looks correct but I will let @onehassan take a look given that something seems broken in the CI and I am not sure what that's about.
@dbarrosop Sorry I haven't checked the details of failing tests. I'll update them.
Tests seems to pass now, i'm not sure about the score-package-quality
though.
Thank you @totzk9 for the PR π , I'm going to add a quick fix commit and then we will release a new version
PR Type
Enhancement, Bug fix, Tests
Description
emailVerified
,phoneNumber
, andphoneNumberVerified
fields to theUser
class inauth_api_types.dart
.fromJson
,toJson
, andtoString
methods in theUser
class to handle the new fields.createTestUser
function intest_helpers.dart
.other
parameter in the equality operator inprovider.dart
.Changes walkthrough π
test_helpers.dart
Add missing fields to test user creation function
packages/nhost_dart/test/test_helpers.dart
emailVerified
,phoneNumber
, andphoneNumberVerified
fields tothe
createTestUser
function.provider.dart
Fix lint issue by changing parameter type in equality operator
packages/nhost_flutter_auth/lib/src/provider.dart
other
parameter in the equality operator fromdynamic
toObject
.auth_api_types.dart
Add new fields to User class and update serialization methods
packages/nhost_sdk/lib/src/api/auth_api_types.dart
emailVerified
,phoneNumber
, andphoneNumberVerified
fields tothe
User
class.fromJson
method to handle the new fields.toJson
method to include the new fields.toString
method to include the new fields.