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

PrintReportButtonError #321

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

PrintReportButtonError #321

wants to merge 7 commits into from

Conversation

DaveLuhman
Copy link
Owner

@DaveLuhman DaveLuhman commented Jan 18, 2025

  • Updated the getCheckedInTools function to use a consistent casing for "Stockroom".
  • Improved the createTool function to check for duplicates individually, providing detailed error messages for each duplicate found.
  • Enhanced error handling in the createTool function to return a structured error response with a list of duplicates.
  • Updated the getRecentlyUpdatedTools function to use a more generic variable name for better clarity.
  • Integrated initCachedContent middleware into several routes to ensure cached data is utilized effectively.

These changes improve the maintainability, clarity, and user experience of the tool management features.

Summary by Sourcery

Enhance tool management features by improving duplicate handling and error reporting in the createTool function, ensuring consistent data retrieval in getCheckedInTools, and integrating caching middleware for better performance.

Bug Fixes:

  • Correct the casing of 'Stockroom' in the getCheckedInTools function to ensure consistent data retrieval.

Enhancements:

  • Improve the createTool function to handle duplicate checks individually and provide detailed error messages for each duplicate found.
  • Enhance error handling in the createTool function to return a structured error response with a list of duplicates.
  • Update the getRecentlyUpdatedTools function to use a more generic variable name for better clarity.
  • Integrate initCachedContent middleware into several routes to ensure effective use of cached data.

This change is Reviewable

- Removed unnecessary whitespace in the `_printerFriendly.hbs` file for cleaner code.
- Updated the `_recentlyUpdated.hbs` file to replace legacy variable names and streamline the rendering logic.

These changes enhance the maintainability and readability of the Handlebars templates.
- Added a new route to fetch cached data in `dashboard.routes.js`, improving data retrieval efficiency.
- Updated the Handlebars template to use a more generic variable name (`tools` instead of `recentlyUpdatedTools`) for better clarity and consistency.
- Modified meta tags in `main.hbs` to reflect updated branding and improve SEO, including changes to the description and canonical link.

These changes enhance the user experience and maintainability of the dashboard component.
- Updated the `getCheckedInTools` function to use a consistent casing for "Stockroom".
- Improved the `createTool` function to check for duplicates individually, providing detailed error messages for each duplicate found.
- Enhanced error handling in the `createTool` function to return a structured error response with a list of duplicates.
- Updated the `getRecentlyUpdatedTools` function to use a more generic variable name for better clarity.
- Integrated `initCachedContent` middleware into several routes to ensure cached data is utilized effectively.

These changes improve the maintainability, clarity, and user experience of the tool management features.
Copy link

sourcery-ai bot commented Jan 18, 2025

Reviewer's Guide by Sourcery

This pull request enhances the tool management features by improving duplicate checking and error handling in the createTool function, ensuring consistent casing in getCheckedInTools, and integrating caching middleware across routes. It also updates variable names for clarity, refines caching logic in toolkeeper.js, and updates meta tags in main.hbs.

Sequence diagram for enhanced tool creation with duplicate checking

sequenceDiagram
    participant C as Client
    participant S as Server
    participant DB as Database

    C->>S: POST /tool/submit
    activate S
    S->>DB: Check serialNumber
    DB-->>S: Return matching tool
    S->>DB: Check barcode
    DB-->>S: Return matching tool
    S->>DB: Check toolID
    DB-->>S: Return matching tool
    alt Duplicates found
        S-->>C: Return error list with duplicate details
    else No duplicates
        S->>DB: Create new tool
        DB-->>S: Return created tool
        S->>S: Update cache
        S-->>C: Redirect to dashboard
    end
    deactivate S
Loading

Class diagram for Tool error handling structure

classDiagram
    class Error {
        +message: string
        +errorList: DuplicateError[]
    }

    class DuplicateError {
        +cause: string
        +duplicateValue: string
        +existingTool: string
    }

    Error "1" -- "*" DuplicateError

    note for DuplicateError "New structured error format for duplicate tools"
Loading

File-Level Changes

Change Details Files
Updated getCheckedInTools function for consistent casing
  • Changed 'stockroom' to 'Stockroom' in the query condition
src/middleware/tool.js
Improved createTool function for duplicate checking and error handling
  • Implemented individual checks for duplicates based on serial number, barcode, and tool ID
  • Created a structured error response with a list of duplicates
  • Modified error handling to render results with error details
src/middleware/tool.js
Updated getRecentlyUpdatedTools function for clarity
  • Changed variable name from 'recentlyUpdatedTools' to 'tools' for better clarity
src/middleware/tool.js
src/views/partials/dashboard/_recentlyUpdated.hbs
src/views/dashboard.hbs
Integrated initCachedContent middleware into routes
  • Added initCachedContent middleware to several routes to ensure cached data is utilized
src/routes/tool.routes.js
src/routes/dashboard.routes.js
Enhanced caching mechanism in toolkeeper.js
  • Added logic to fetch and update cache if data is stale
  • Implemented error handling for cache fetching
  • Updated dashboard population logic to use cached data
src/public/js/toolkeeper.js
Updated meta tags and script loading in main.hbs
  • Changed meta description and author
  • Updated canonical link
  • Modified script tag to load toolkeeper.js as a module
src/views/layouts/main.hbs
Removed unused btnToSpinner function
  • Deleted the btnToSpinner function from toolkeeper.js
src/public/js/toolkeeper.js

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @DaveLuhman - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

  • The use of globalThis.open() could be vulnerable to cross-site scripting attacks. (link)
  • Using innerHTML with template literals could pose an XSS risk if data contains malicious content. (link)

Overall Comments:

  • The print functionality appears to have been broken by removing the onclick handler for openInNewTab() in _recentlyUpdated.hbs. Please restore this functionality.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🔴 Security: 2 blocking issues
  • 🟢 Review instructions: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

src/public/js/toolkeeper.js Show resolved Hide resolved
src/public/js/toolkeeper.js Outdated Show resolved Hide resolved
src/public/js/toolkeeper.js Outdated Show resolved Hide resolved
src/middleware/tool.js Outdated Show resolved Hide resolved
src/middleware/tool.js Fixed Show fixed Hide fixed
src/middleware/tool.js Fixed Show fixed Hide fixed
src/middleware/tool.js Fixed Show fixed Hide fixed
- Updated the `/disableUser/:id` route to include `getUsers` middleware, ensuring user data is refreshed after disabling a user.
- Modified the `/:id/delete` route to also include `getUsers`, improving the user list consistency after a deletion.

These changes improve the user management experience and maintain the clarity of route functionalities.
- Simplified the duplicate checking process by consolidating queries into a single database call, improving performance and readability.
- Enhanced error handling to provide a structured response with detailed information about duplicate tools found, including field names and values.
- Removed individual duplicate checks for serial number, barcode, and tool ID, streamlining the code.

These changes improve the maintainability and efficiency of the tool creation process.
- Updated the `openInNewTab` function to open a new window with enhanced security features, including a Content Security Policy and removal of script tags from the printed content.
- Refactored the dashboard rendering logic to create links for service assignments, improving clarity and maintainability by using `textContent` instead of `innerHTML`.

These changes improve the user experience and security of the printing feature while enhancing the readability of the dashboard code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant