Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add missing emailVerified, phoneNumber and phoneNumberVerified fields #140

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

totzk9
Copy link
Contributor

@totzk9 totzk9 commented Jul 13, 2024

PR Type

Enhancement, Bug fix, Tests


Description

  • Added emailVerified, phoneNumber, and phoneNumberVerified fields to the User class in auth_api_types.dart.
  • Updated the fromJson, toJson, and toString methods in the User class to handle the new fields.
  • Added the new fields to the createTestUser function in test_helpers.dart.
  • Fixed a lint issue by changing the type of the other parameter in the equality operator in provider.dart.

Changes walkthrough 📝

Relevant files
Tests
test_helpers.dart
Add missing fields to test user creation function               

packages/nhost_dart/test/test_helpers.dart

  • Added emailVerified, phoneNumber, and phoneNumberVerified fields to
    the createTestUser function.
  • +3/-0     
    Bug fix
    provider.dart
    Fix lint issue by changing parameter type in equality operator

    packages/nhost_flutter_auth/lib/src/provider.dart

  • Changed the type of the other parameter in the equality operator from
    dynamic to Object.
  • +1/-1     
    Enhancement
    auth_api_types.dart
    Add new fields to User class and update serialization methods

    packages/nhost_sdk/lib/src/api/auth_api_types.dart

  • Added emailVerified, phoneNumber, and phoneNumberVerified fields to
    the User class.
  • Updated the fromJson method to handle the new fields.
  • Updated the toJson method to include the new fields.
  • Updated the toString method to include the new fields.
  • +15/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @qodo-merge-pro qodo-merge-pro bot added the enhancement New feature or request label Jul 13, 2024
    Copy link

    qodo-merge-pro bot commented Jul 13, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit bd9214c)

    ⏱️ 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.

    Copy link

    qodo-merge-pro bot commented Jul 13, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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:

    -      phoneNumber: json['phoneNumber'] as String,
    +      phoneNumber: json['phoneNumber'] ?? '',

    Consider adding null checks for the new fields 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]

    -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]: 9

    Why: 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 emailVerified and phoneNumberVerified are consistent with email and phone number fields

    To ensure data consistency, consider implementing checks or constraints on the
    emailVerified and phoneNumberVerified fields to ensure they are not set to true if
    email or phoneNumber fields are empty or null respectively.

    packages/nhost_sdk/lib/src/api/auth_api_types.dart [119-121]

     final bool emailVerified;
     final bool phoneNumberVerified;
     
    +// Ensure these fields are checked for consistency elsewhere in your code.
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Ensuring consistency between verification fields and their corresponding data fields is important for data integrity and preventing logical errors.

    8
    Enhancement
    Validate the phoneNumber field to ensure it contains a valid phone number

    Ensure that the phoneNumber field is validated to check if it conforms to a standard
    phone number format. This can be done using a regular expression or a dedicated
    library for phone number validation.

    packages/nhost_sdk/lib/src/api/auth_api_types.dart [120]

    -final String phoneNumber;
    +final String phoneNumber; // Ensure to validate this field elsewhere in your code.
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Ensuring that the phoneNumber field is validated is important for data integrity, but the suggestion to add a comment for validation elsewhere is not a direct code improvement.

    7
    Maintainability
    Refactor boolean to string conversion into a reusable method

    For better maintainability and to avoid redundancy, consider creating a method that
    handles 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]

    -'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]: 6

    Why: Creating a reusable method for boolean to string conversion can improve maintainability, but it is a minor enhancement and not critical for functionality.

    6

    @dbarrosop
    Copy link
    Member

    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.

    @totzk9
    Copy link
    Contributor Author

    totzk9 commented Jul 15, 2024

    @dbarrosop
    Sorry I haven't checked the details of failing tests. I'll update them.

    @qodo-merge-pro qodo-merge-pro bot added the Tests label Jul 15, 2024
    @totzk9
    Copy link
    Contributor Author

    totzk9 commented Jul 15, 2024

    Tests seems to pass now, i'm not sure about the score-package-quality though.

    @totzk9
    Copy link
    Contributor Author

    totzk9 commented Jul 15, 2024

    CI checks are now successful after fixing a linter warning 49cde90

    @onehassan
    Copy link
    Contributor

    Thank you @totzk9 for the PR 🙌 , I'm going to add a quick fix commit and then we will release a new version

    @onehassan onehassan merged commit adc184e into nhost:main Jul 19, 2024
    19 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants