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-1772] fix marketplace listing #111

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented May 22, 2024

PR Type

Enhancement, Bug fix


Description

  • Updated Chip.vue to fix background color class binding for non-error states.
  • Modified CollapseFilter.vue to use dynamic theme from useAppStore.
  • Added --popper-theme-padding CSS variable in Popover.vue.
  • Enhanced CreateListing.vue by adding auction functionality, removing the account field, and updating validation schema.
  • Updated marketplace.ts to remove account from createListing and added getCurrentBlock method.
  • Added GetBlocks query to queries.ts and GetBlocks.ts.
  • Removed account parameter from CreateListing mutation.

Changes walkthrough 📝

Relevant files
Bug fix
Chip.vue
Update class binding for non-error state background colors

resources/js/components/Chip.vue

  • Updated class binding for non-error state to use
    bg-light-surface-background and bg-dark-surface-background.
  • +1/-1     
    Enhancement
    CollapseFilter.vue
    Use dynamic theme from store in Popover component               

    resources/js/components/CollapseFilter.vue

  • Changed theme binding to use value from useAppStore.
  • Added import for useAppStore.
  • +2/-1     
    Popover.vue
    Add popper theme padding variable in light mode                   

    resources/js/components/Popover.vue

  • Added --popper-theme-padding CSS variable with value 0 in light mode.
  • +1/-0     
    CreateListing.vue
    Add auction functionality and remove account field in CreateListing

    resources/js/components/pages/create/CreateListing.vue

  • Removed account field from the form.
  • Added enableAuction checkbox and conditional auction fields.
  • Updated validation schema to handle conditional fields.
  • Added getCurrentBlock method to fetch the current block number.
  • Added watcher for enableAuction to reset auction fields when toggled.
  • +74/-35 
    marketplace.ts
    Update createListing method and add getCurrentBlock method

    resources/js/api/marketplace.ts

  • Removed account from createListing method.
  • Added getCurrentBlock method to fetch the current block number.
  • +11/-1   
    queries.ts
    Add GetBlocks query to exports                                                     

    resources/js/api/queries.ts

    • Added GetBlocks query to the export list.
    +2/-0     
    CreateListing.ts
    Remove account parameter from CreateListing mutation         

    resources/js/graphql/mutation/marketplace/CreateListing.ts

    • Removed account parameter from CreateListing mutation.
    +1/-2     
    GetBlocks.ts
    Add GetBlocks query to fetch block numbers                             

    resources/js/graphql/query/marketplace/GetBlocks.ts

    • Added new query GetBlocks to fetch block numbers.
    +9/-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 22, 2024
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels May 22, 2024
    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and components with both frontend and backend changes, including Vue components, API service modifications, and GraphQL queries. The complexity of the changes, especially with the addition of auction functionality, requires careful review to ensure functionality and integration.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The getCurrentBlock method in CreateListing.vue does not handle the case where the GetBlocks query might return an empty array, which would cause an error when trying to access res.data.GetBlocks.edges[0].node.number.

    Data Validation Issue: The validation schema in CreateListing.vue might not properly handle cases where enableAuction is false but auctionDataStart and auctionDataEnd fields are still provided by the user, leading to potential inconsistent data submission.

    🔒 Security concerns

    No

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

    Consider adding error handling for the case where the GetBlocks query returns an empty array to prevent runtime errors. This can be done by checking if res.data.GetBlocks.edges.length > 0 before setting auctionDataStart.value. [important]

    relevant lineauctionDataStart.value = res.data?.GetBlocks?.edges[0].node.number;

    relevant fileresources/js/components/pages/create/CreateListing.vue
    suggestion      

    Add a validation condition to ensure that auctionDataStart and auctionDataEnd are not provided when enableAuction is false. This can be done by enhancing the conditional validation in the Yup schema to ignore these fields if the auction is not enabled. [important]

    relevant lineauctionDataStart: yup.string().when('enableAuction', {

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add validation to ensure auction start and end blocks are set in the future

    Consider adding a validation check for auctionDataStart and auctionDataEnd to ensure they
    are not set to past block numbers when enableAuction is true. This will prevent users from
    accidentally setting an auction with invalid start or end blocks.

    resources/js/components/pages/create/CreateListing.vue [220-229]

     auctionDataStart: yup.string().when('enableAuction', {
         is: true,
    -    then: () => numberRequiredSchema.typeError('Auction Start Block must be a number'),
    +    then: () => numberRequiredSchema.min(currentBlock, 'Auction Start Block must be in the future').typeError('Auction Start Block must be a number'),
         otherwise: () => yup.string().notRequired(),
     }),
     auctionDataEnd: yup.string().when('enableAuction', {
         is: true,
    -    then: () => numberRequiredSchema.typeError('Auction End Block must be a number'),
    +    then: () => numberRequiredSchema.min(currentBlock, 'Auction End Block must be in the future').typeError('Auction End Block must be a number'),
         otherwise: () => yup.string().notRequired(),
     }),
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential issue where users could set auction start and end blocks to past values, which would be invalid. Adding this validation improves the robustness of the application.

    9
    Modify the watch function to clear auction data fields immediately when the auction is disabled

    To improve the user experience and avoid unnecessary API calls, consider disabling the
    getCurrentBlock function call when the auction is disabled, and clear the auction data
    fields immediately.

    resources/js/components/pages/create/CreateListing.vue [300-310]

     watch(
         () => enableAuction.value,
         () => {
             if (enableAuction.value) {
                 getCurrentBlock();
             } else {
                 auctionDataStart.value = '';
                 auctionDataEnd.value = '';
    +            // Immediately clear any previously fetched block data to prevent confusion
    +            auctionDataStart.value = null;
    +            auctionDataEnd.value = null;
             }
         }
     );
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the user experience by ensuring that auction data fields are cleared immediately when the auction is disabled, preventing any confusion or unnecessary API calls.

    7
    Possible issue
    Improve error handling in the getCurrentBlock function to handle empty or invalid responses

    Implement error handling for the getCurrentBlock function to manage cases where the API
    call fails, preventing the application from crashing and providing feedback to the user.

    resources/js/components/pages/create/CreateListing.vue [238-247]

     const getCurrentBlock = async () => {
         try {
             const res = await MarketplaceApi.getCurrentBlock();
    -        auctionDataStart.value = res.data?.GetBlocks?.edges[0].node.number;
    +        if (res.data && res.data.GetBlocks && res.data.GetBlocks.edges.length > 0) {
    +            auctionDataStart.value = res.data.GetBlocks.edges[0].node.number;
    +        } else {
    +            throw new Error('No block data available');
    +        }
         } catch (e) {
             snackbar.error({
                 title: 'Failed to get current block',
    -            text: 'Please try again.',
    +            text: 'No block data available. Please try again.',
             });
         }
     };
     
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances the error handling of the getCurrentBlock function, ensuring that the application can gracefully handle cases where the API response is empty or invalid, thus improving user experience and reliability.

    8
    Possible bug
    Ensure auction data is only included in the request payload when all required fields are valid

    Refactor the createListing function to handle the case where enableAuction is false more
    gracefully by ensuring that the auction data is not included in the request payload, thus
    avoiding potential errors on the server side.

    resources/js/components/pages/create/CreateListing.vue [269-274]

    -auctionData: enableAuction.value
    +auctionData: enableAuction.value && auctionDataStart.value && auctionDataEnd.value
         ? {
               startBlock: auctionDataStart.value,
               endBlock: auctionDataEnd.value,
           }
    -    : null,
    +    : undefined,
     
    Suggestion importance[1-10]: 8

    Why: This suggestion prevents potential errors on the server side by ensuring that auction data is only included in the request payload when all required fields are valid, thus improving the robustness of the createListing function.

    8

    @zlayine zlayine merged commit 580e254 into master May 23, 2024
    3 checks passed
    @zlayine zlayine deleted the bugfix/pla-1772/listings branch May 23, 2024 03:40
    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