Skip to content

Conversation

@zlayine
Copy link
Contributor

@zlayine zlayine commented Jan 21, 2025

PR Type

Enhancement, Bug fix


Description

  • Refactored fuel tank creation and mutation logic, introducing coveragePolicy and removing outdated flags.

  • Enhanced UI components for better usability, including FormCheckbox, RadioInput, and MultiCheckbox.

  • Updated GraphQL queries and mutations to align with new fuel tank attributes.

  • Added new transaction methods and enums to support expanded functionality.


Changes walkthrough 📝

Relevant files
Enhancement
16 files
FormCheckbox.vue
Improved checkbox component with new props and styles       
+69/-13 
FormSelect.vue
Removed unnecessary lifecycle hook for select component   
+1/-5     
InputHeader.vue
Simplified and styled input header component                         
+3/-6     
MultiCheckbox.vue
Updated multi-checkbox component with new styles and logic
+35/-18 
RadioGroupButton.vue
Adjusted radio group button styling                                           
+1/-1     
RadioInput.vue
Added new radio input component for selection                       
+81/-0   
SupportButton.vue
Updated support button icon styling                                           
+1/-1     
CreateFuelTank.vue
Refactored fuel tank creation form with new fields             
+52/-43 
DetailsFuelTankSlideover.vue
Updated fuel tank details to include new attributes           
+12/-16 
DispatchFuelTankSlideover.vue
Added dispatch signature fields to dispatch slideover       
+19/-2   
MutateFuelTankSlideover.vue
Refactored fuel tank mutation form with new logic               
+66/-32 
fueltank.ts
Updated API methods for fuel tank creation and mutation   
+4/-2     
CreateFuelTank.ts
Refactored GraphQL mutation for creating fuel tanks           
+4/-3     
GetFuelTank.ts
Updated GraphQL query to include new fuel tank attributes
+2/-0     
GetFuelTanks.ts
Enhanced GraphQL query for fetching multiple fuel tanks   
+2/-3     
types.enums.ts
Added new transaction methods and enums for fuel tanks     
+26/-1   

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The onChange method in the FormCheckbox component does not account for potential edge cases where localModelValue might not toggle correctly. This could lead to unexpected behavior.

const onChange = () => {
    if (props.disabled) {
        return;
    }

    localModelValue.value = !localModelValue.value;
};

watch(
    () => props.disabled,
    (value) => {
        if (value) {
            localModelValue.value = false;
        }
    }
);
Validation Logic

The validation schema for coveragePolicy, requireAccount, and reserveAccountCreationDeposit in the CreateFuelTank component might not sufficiently handle invalid or unexpected input values.

    name: yup.string().nullable().required(),
    coveragePolicy: yup.string().nullable(),
    accountManagement: yup.boolean().nullable(),
    reserveAccountCreationDeposit: yup.boolean().nullable(),
    requireAccount: yup.boolean().nullable(),
    signingAccount: yup.string().nullable(),
});
API Mutation Parameters

The mutateFuelTank and createFuelTank methods in the FuelTankApi class introduce new parameters like coveragePolicy and signingAccount. Ensure these are correctly handled and validated on the backend.

static async mutateFuelTank(mutateFuelTankData: Record<string, unknown>) {
    const data = {
        query: mutations.MutateFuelTank,
        variables: {
            tankId: mutateFuelTankData.tankId,
            mutation: mutateFuelTankData.mutation,
            signingAccount: mutateFuelTankData.signingAccount,
            idempotencyKey: mutateFuelTankData.idempotencyKey,

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Prevent toggling when disabled

Ensure that the onChange method properly toggles the localModelValue only when the
disabled prop is false, as improper handling might lead to unexpected behavior.

resources/js/components/FormCheckbox.vue [35]

-@click="onChange"
+@click="!disabled && onChange"
Suggestion importance[1-10]: 8

Why: The suggestion ensures that the onChange method does not execute when the disabled prop is true, preventing unintended toggling behavior. This is a critical fix for maintaining proper functionality and user experience.

8
Validate coveragePolicy before mutation

Add a check to ensure the coveragePolicy value is valid before proceeding with the
mutation to prevent invalid API requests.

resources/js/components/slideovers/fueltank/MutateFuelTankSlideover.vue [252]

-coveragePolicy: coveragePolicy.value,
+coveragePolicy: coveragePolicy.value || 'FEES',
Suggestion importance[1-10]: 7

Why: This suggestion ensures that a valid coveragePolicy value is always passed during the mutation, preventing potential API errors. It is a useful enhancement for maintaining data integrity.

7
General
Validate critical form fields

Validate that coveragePolicy and signingAccount are correctly set before submitting
the form to avoid potential API errors or unexpected behavior.

resources/js/components/pages/create/CreateFuelTank.vue [228-229]

 const coveragePolicy = ref('FEES');
 const signingAccount = ref('');
+if (!coveragePolicy.value || !signingAccount.value) {
+    throw new Error('Coverage policy and signing account must be set.');
+}
Suggestion importance[1-10]: 6

Why: Adding validation for coveragePolicy and signingAccount ensures that these critical fields are set before form submission, reducing the risk of API errors. However, the implementation could be improved by integrating this validation into the existing form validation logic.

6
Set explicit defaults for mutation

Ensure that the coveragePolicy and requireAccount default values are explicitly set
in the mutation to avoid unexpected behavior if these fields are omitted.

resources/js/graphql/mutation/fueltank/CreateFuelTank.ts [1]

-$coveragePolicy: CoveragePolicy = FEES, $requireAccount: Boolean = false
+$coveragePolicy: CoveragePolicy = FEES, $requireAccount: Boolean = false, $signingAccount: String = ""
Suggestion importance[1-10]: 5

Why: Explicitly setting default values for coveragePolicy, requireAccount, and signingAccount in the mutation improves clarity and prevents unexpected behavior. However, the addition of signingAccount as an empty string default may not be necessary or appropriate depending on the API's requirements.

5

@zlayine zlayine changed the title [PLA-2149] fueltank fix [PLA-2149] [PLA-1923] fueltank fix Jan 21, 2025
@zlayine zlayine merged commit 3b8cad6 into master Jan 21, 2025
4 checks passed
@zlayine zlayine deleted the bugfix/PLA-2149/fueltank-fix branch January 21, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants