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-1762] form fixes #107

Merged
merged 4 commits into from
May 13, 2024
Merged

[PLA-1762] form fixes #107

merged 4 commits into from
May 13, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented May 13, 2024

PR Type

enhancement, bug_fix


Description

  • Enhanced several Vue components by adding non-null assertions and required validations to improve form handling and prevent runtime errors.
  • Added error handling and UI improvements in DispatchRuleForm and RuleSetsFuelTankSlideover.
  • Updated package.json and package-lock.json to include lodash as a dependency.

Changes walkthrough 📝

Relevant files
Enhancement
4 files
FormSelect.vue
Refactor FormSelect component for improved usability         

resources/js/components/FormSelect.vue

  • Removed dynamic class binding and added static class definition for
    better predictability.
  • Added autocomplete="off" to disable browser autocomplete.
  • +3/-2     
    DispatchRuleForm.vue
    Enhance DispatchRuleForm with error handling and UI updates

    resources/js/components/fueltank/DispatchRuleForm.vue

  • Added UI elements to indicate required fields and display errors.
  • Removed unused imports and updated validation schemas.
  • Added clearSelectedDispatchRule function to reset form state based on
    rule type.
  • +57/-15 
    DispatchFuelTankSlideover.vue
    Enhance form validation in DispatchFuelTankSlideover         

    resources/js/components/slideovers/fueltank/DispatchFuelTankSlideover.vue

  • Added required attribute to query input and updated tankId to use
    non-null assertion.
  • +4/-2     
    RuleSetsFuelTankSlideover.vue
    Enhance error handling in RuleSetsFuelTankSlideover           

    resources/js/components/slideovers/fueltank/RuleSetsFuelTankSlideover.vue

    • Added error handling and required attribute to DispatchRuleForm.
    +15/-2   
    Bug_fix
    10 files
    BatchMintForm.vue
    Ensure non-null values in BatchMintForm component               

    resources/js/components/batch/forms/BatchMintForm.vue

  • Changed account ref initialization to ensure non-null values using
    TypeScript non-null assertion operator.
  • Updated beneficiaryAddress to ensure non-null values.
  • +2/-2     
    BatchTransferForm.vue
    Update non-null assertions in BatchTransferForm                   

    resources/js/components/batch/forms/BatchTransferForm.vue

  • Updated account and operatorSource refs to use non-null assertion for
    initialization.
  • +2/-2     
    ClaimBeamSlideover.vue
    Fix clipboard copy in ClaimBeamSlideover                                 

    resources/js/components/slideovers/beam/ClaimBeamSlideover.vue

    • Updated clipboard copy functionality to use non-null assertion.
    +1/-1     
    UpdateBeamSlideover.vue
    Update date handling in UpdateBeamSlideover                           

    resources/js/components/slideovers/beam/UpdateBeamSlideover.vue

  • Updated date comparison to use non-null assertion for start and end
    dates.
  • +2/-2     
    DetailsTransactionSlideover.vue
    Fix transaction retrieval in DetailsTransactionSlideover 

    resources/js/components/slideovers/common/DetailsTransactionSlideover.vue

  • Updated transaction retrieval to use non-null assertion for
    transaction ID.
  • +1/-1     
    FreezeSlideover.vue
    Update tokenId handling in FreezeSlideover                             

    resources/js/components/slideovers/common/FreezeSlideover.vue

    • Updated tokenId initialization to use non-null assertion.
    +1/-1     
    AccountsFuelTankSlideover.vue
    Fix tankId handling in AccountsFuelTankSlideover                 

    resources/js/components/slideovers/fueltank/AccountsFuelTankSlideover.vue

    • Updated tankId to use non-null assertion for API call.
    +2/-2     
    ConsumptionFuelTankSlideover.vue
    Fix tankId handling in ConsumptionFuelTankSlideover           

    resources/js/components/slideovers/fueltank/ConsumptionFuelTankSlideover.vue

  • Updated tankId to use non-null assertion in setConsumption API call.
  • +1/-1     
    FreezeFuelTankSlideover.vue
    Fix tankId handling in FreezeFuelTankSlideover                     

    resources/js/components/slideovers/fueltank/FreezeFuelTankSlideover.vue

  • Updated tankId to use non-null assertion in scheduleMutateFreezeState
    API call.
  • +1/-1     
    MutateFuelTankSlideover.vue
    Fix tankId handling in MutateFuelTankSlideover                     

    resources/js/components/slideovers/fueltank/MutateFuelTankSlideover.vue

  • Updated tankId to use non-null assertion in mutateFuelTank API call.
  • +1/-1     
    Dependencies
    2 files
    package-lock.json
    Add lodash dependency                                                                       

    package-lock.json

    • Added lodash library version 4.17.21.
    +1/-0     
    package.json
    Update package.json to include lodash                                       

    package.json

    • Added lodash to dependencies.
    +1/-0     

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

    Copy link

    PR Description updated to latest commit (b8676cb)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files with changes that include both UI and backend logic enhancements. The use of non-null assertions and updates to form handling requires careful consideration to ensure that no runtime errors are introduced.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The use of non-null assertions (!) in Vue components, such as in publicKeyToAddress(props.modelValue.account!)!, could lead to runtime errors if account is actually null or undefined.

    Data Integrity: The removal of certain utility functions like formatToken and changes in validation schemas might affect how data is processed and validated, potentially leading to incorrect data handling.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileresources/js/components/fueltank/DispatchRuleForm.vue
    suggestion      

    Consider adding a fallback or handling for null values when using non-null assertions to prevent potential runtime errors. For example, you could modify the non-null assertion to a conditional check that provides a default value or handles the null case gracefully. [important]

    relevant linecollectionId: ref(props.modelValue.requireToken?.collectionId!);

    relevant fileresources/js/components/slideovers/fueltank/RuleSetsFuelTankSlideover.vue
    suggestion      

    Implement a more robust error handling or user feedback mechanism when dispatchRule is empty, to enhance user experience and prevent confusion during form submissions. [important]

    relevant linedispatchRuleError.value = true;

    relevant fileresources/js/components/FormSelect.vue
    suggestion      

    Ensure that removing dynamic class binding and replacing it with a static class does not remove needed responsiveness or adaptability of the component across different devices or conditions. If dynamic classes were handling specific cases, consider reintroducing them conditionally. [medium]

    relevant lineclass="w-full py-2.5 pl-3 pr-10 text-sm leading-5 border-0 text-gray-900 ring-1 ring-inset ring-gray-300 placeholder:text-gray-400 focus:ring-2 focus:ring-inset focus:ring-primary rounded-md transition-all disabled:bg-gray-100"

    relevant fileresources/js/components/slideovers/fueltank/DispatchFuelTankSlideover.vue
    suggestion      

    For the query field in the form, since it's marked as required, ensure that the backend appropriately handles cases where query might still be an empty string, to prevent execution of empty or invalid queries. [important]

    relevant linequery: stringRequiredSchema,

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for potentially undefined inputs

    Ensure proper error handling for the publicKeyToAddress function to avoid runtime errors
    if props.modelValue.account is undefined or invalid.

    resources/js/components/batch/forms/BatchMintForm.vue [247]

    -const account = ref(publicKeyToAddress(props.modelValue.account!)!);
    +const account = ref(props.modelValue.account ? publicKeyToAddress(props.modelValue.account) : null);
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential bug where the publicKeyToAddress function might receive an undefined value, leading to runtime errors. Adding error handling improves robustness.

    8
    Add a check to ensure payload exists before copying to clipboard

    Check for the existence of props.item?.qr.payload before attempting to copy to clipboard
    to prevent potential errors when the payload is undefined.

    resources/js/components/slideovers/beam/ClaimBeamSlideover.vue [52]

    -navigator.clipboard.writeText(props.item?.qr.payload!);
    +if (props.item?.qr.payload) {
    +    navigator.clipboard.writeText(props.item.qr.payload);
    +} else {
    +    console.error('Payload is undefined');
    +}
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly addresses a potential bug where the payload might be undefined, which would cause an error when attempting to copy to the clipboard. Adding a check before copying enhances error handling.

    8
    Add a null check for tankId.value to prevent runtime errors

    Consider adding a check for tankId.value before using it in addressToPublicKey. This will
    prevent potential runtime errors if tankId.value is null or undefined.

    resources/js/components/slideovers/fueltank/RuleSetsFuelTankSlideover.vue [192]

    -tankId: addressToPublicKey(tankId.value!),
    +tankId: tankId.value ? addressToPublicKey(tankId.value) : null,
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential runtime error due to the lack of null checking before using tankId.value in a function that expects a non-null value. This is a crucial fix to prevent crashes.

    8
    Enhancement
    Verify backend validation for ruleSetId aligns with the type="number" attribute

    The addition of the type="number" attribute in the FormInput component for ruleSetId might
    imply that the input should strictly be a number. Ensure that the backend also validates
    this assumption to prevent type mismatches.

    resources/js/components/slideovers/fueltank/RuleSetsFuelTankSlideover.vue [42]

    -type="number"
    +type="number"  // Ensure backend validation aligns with this type
     
    Suggestion importance[1-10]: 7

    Why: Ensuring that the backend validation aligns with frontend assumptions is crucial for data integrity and user experience. This suggestion is important to prevent possible type mismatches and related bugs.

    7
    Performance
    Use native JavaScript to check if dispatchRule.value is empty, reducing dependency on lodash

    Instead of using lodash's isEmpty for checking if dispatchRule.value is empty, consider
    using native JavaScript functionality to reduce dependency overhead.

    resources/js/components/slideovers/fueltank/RuleSetsFuelTankSlideover.vue [178]

    -if (isEmpty(dispatchRule.value)) {
    +if (!dispatchRule.value || Object.keys(dispatchRule.value).length === 0) {
     
    Suggestion importance[1-10]: 6

    Why: This suggestion promotes reducing dependency overhead by using native JavaScript, which is a good practice for performance and simplicity. However, it's not a critical issue, hence the moderate score.

    6
    Possible issue
    Ensure the required attribute is properly implemented in the component

    The required attribute on the component might not be effective as it's a custom Vue
    component. Ensure that the component internally handles the required validation or
    consider implementing a custom validation method.

    resources/js/components/slideovers/fueltank/RuleSetsFuelTankSlideover.vue [59]

    -required
    +:required="true"
     
    Suggestion importance[1-10]: 5

    Why: The suggestion addresses a potential issue with custom Vue components handling of required attributes, which is relevant for form validation. However, without specific details on how the component handles props, the impact of this suggestion is uncertain.

    5

    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.

    remove debug

    permittedExtrinsics: permittedExtrinsics.value,
    })
    );

    watch(
    () => hasChanged.value,
    async () => {
    console.log(hasChanged.value);
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    debug here

    @zlayine zlayine force-pushed the bugfix/pla-1762/form-fixes branch from 92a6149 to bf142bd Compare May 13, 2024 13:49
    @zlayine zlayine requested a review from enjinabner May 13, 2024 13:50
    @zlayine zlayine merged commit dd58f21 into master May 13, 2024
    3 checks passed
    @zlayine zlayine deleted the bugfix/pla-1762/form-fixes branch May 13, 2024 13:55
    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.

    2 participants