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

[PLA-1718] add email change #109

Merged
merged 2 commits into from
May 13, 2024
Merged

[PLA-1718] add email change #109

merged 2 commits into from
May 13, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented May 13, 2024

PR Type

enhancement


Description

  • Added a new Vue component SettingsChangeEmail for email change functionality.
  • Integrated SettingsChangeEmail into Settings.vue with conditions based on multi-tenancy.
  • Refactored Settings.vue to use a new computed property isMultiTenant for cleaner condition checks.
  • Implemented a new API method updateUser in auth.ts to handle email updates.
  • Defined a new GraphQL mutation UpdateUser to support the email update functionality.

Changes walkthrough 📝

Relevant files
Enhancement
Settings.vue
Integrate SettingsChangeEmail and Refactor Multi-Tenant Checks

resources/js/components/pages/Settings.vue

  • Simplified conditions by using the new isMultiTenant computed
    property.
  • Added a new component SettingsChangeEmail for changing email
    functionality.
  • Updated button conditions and actions to align with the new
    isMultiTenant logic.
  • +9/-7     
    SettingsChangeEmail.vue
    New Component for Email Change Functionality                         

    resources/js/components/pages/SettingsChangeEmail.vue

  • Created a new Vue component for changing user email.
  • Includes form handling, validation, and submission logic.
  • Utilizes existing components and API for updating user email.
  • +89/-0   
    auth.ts
    Add API Method for Email Update                                                   

    resources/js/api/auth.ts

    • Added a new method updateUser to handle email updates via API.
    +11/-0   
    mutations.ts
    Add GraphQL Mutation for Email Update                                       

    resources/js/api/mutations.ts

    • Added a new GraphQL mutation UpdateUser for updating user email.
    +2/-0     
    UpdateUser.ts
    Define GraphQL Mutation for Updating User Email                   

    resources/js/graphql/mutation/auth/UpdateUser.ts

    • Defined a new GraphQL mutation for updating user email.
    +3/-0     

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

    @zlayine zlayine self-assigned this May 13, 2024
    @github-actions github-actions bot added the enhancement New feature or request label May 13, 2024
    Copy link

    PR Description updated to latest commit (ffb871a)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and integrates new functionality across the Vue components and API layers, requiring a thorough review of both frontend logic and backend API integration.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The computed property isMultiTenant is incorrectly negating the value of appStore.isMultiTenant. This could lead to incorrect UI behavior where multi-tenant specific features are shown or hidden incorrectly.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileresources/js/components/pages/Settings.vue
    suggestion      

    Correct the logic in the computed property isMultiTenant to accurately reflect the multi-tenant status. Currently, it negates the appStore.isMultiTenant, which seems incorrect. Change it to directly return appStore.isMultiTenant. [important]

    relevant lineconst isMultiTenant = computed(() => !appStore.isMultiTenant);

    relevant fileresources/js/components/pages/SettingsChangeEmail.vue
    suggestion      

    Update the button text from "Change password" to "Change email" in the SettingsChangeEmail.vue component to match the functionality. This will improve UI clarity and user experience. [important]

    relevant lineChange password

    relevant fileresources/js/api/auth.ts
    suggestion      

    Add error handling for the updateUser method in auth.ts. Consider using try-catch blocks to handle potential exceptions and provide meaningful error messages to the frontend. [important]

    relevant linestatic async updateUser(email: string) {

    relevant fileresources/js/graphql/mutation/auth/UpdateUser.ts
    suggestion      

    Ensure that the GraphQL mutation UpdateUser returns more detailed information about the operation result. Currently, it only returns a simple boolean or similar minimal response. Consider returning the updated user object or at least a detailed status message. [medium]

    relevant lineexport default `mutation UpdateUser($email: String) {

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the logic in the computed property isMultiTenant to not negate the value

    The computed property isMultiTenant is negating the value of appStore.isMultiTenant, which
    seems incorrect based on the context of the other components using isMultiTenant directly.
    This could lead to unexpected behavior where multi-tenant checks are supposed to pass but
    fail due to the negation.

    resources/js/components/pages/Settings.vue [109]

    -const isMultiTenant = computed(() => !appStore.isMultiTenant);
    +const isMultiTenant = computed(() => appStore.isMultiTenant);
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies a logic error in the computed property isMultiTenant that negates the value of appStore.isMultiTenant. This error could lead to significant functional issues in multi-tenant checks throughout the application.

    10
    Ensure the condition for displaying the initialization guide uses the correct tenant check

    The condition for displaying the initialization guide div has been modified to check
    isMultiTenant directly instead of appStore.isMultiTenant. If this change was
    unintentional, it might lead to the guide being displayed incorrectly based on the tenant
    configuration.

    resources/js/components/pages/Settings.vue [7]

    -<div v-if="!appStore.hasValidConfig && isMultiTenant && !tokens?.length" class="flex flex-col mb-6 w-full transition-all rounded-md bg-[#0284c7] p-3 text-white">
    +<div v-if="!appStore.hasValidConfig && appStore.isMultiTenant && !tokens?.length" class="flex flex-col mb-6 w-full transition-all rounded-md bg-[#0284c7] p-3 text-white">
     
    Suggestion importance[1-10]: 10

    Why: This suggestion correctly identifies a potential issue with the tenant check in the condition for displaying an initialization guide. This is crucial as it affects the visibility of important user guidance based on tenant configuration.

    10
    Enhancement
    Update the button label to accurately reflect its action as changing the email

    The button label in SettingsChangeEmail.vue incorrectly states "Change password" instead
    of "Change email". This can confuse users who are trying to update their email addresses.

    resources/js/components/pages/SettingsChangeEmail.vue [27]

    -<Btn is-submit primary :loading="isLoading" id="changeEmailSubmitBtn">Change password</Btn>
    +<Btn is-submit primary :loading="isLoading" id="changeEmailSubmitBtn">Change Email</Btn>
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly points out a misleading button label that says "Change password" instead of "Change email". This is a clear and important user interface correction.

    8
    Possible issue
    Review the conditional rendering of SettingsChangeEmail to ensure it aligns with intended visibility rules

    The SettingsChangeEmail component is conditionally rendered based on isMultiTenant. If
    isMultiTenant is intended to control visibility based on tenant configuration, ensure that
    this is the desired behavior as it might unintentionally hide the email change option in
    single-tenant configurations.

    resources/js/components/pages/Settings.vue [56]

    -<SettingsChangeEmail v-if="isMultiTenant" />
    +<SettingsChangeEmail v-if="shouldShowEmailChange" />
     
    Suggestion importance[1-10]: 5

    Why: The suggestion raises a valid concern about the conditional rendering based on isMultiTenant, which might not be appropriate for all configurations. However, without more context on the intended behavior, the impact of this suggestion is uncertain.

    5

    @zlayine zlayine force-pushed the feature/pla-1718/change-email branch from ffb871a to 3aacba5 Compare May 13, 2024 06:54
    Copy link
    Contributor

    @enjinabner enjinabner left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    minor change

    @leonardocustodio
    Copy link
    Member

    There is a lint error. An import not needed

    @enjinabner enjinabner self-requested a review May 13, 2024 13:10
    @zlayine zlayine merged commit 49edaff into master May 13, 2024
    3 checks passed
    @zlayine zlayine deleted the feature/pla-1718/change-email branch May 13, 2024 13:49
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Development

    Successfully merging this pull request may close these issues.

    3 participants