040 Nov 5 Add Swagger for Vendors #206
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to the vendor management functionality within the application. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
Servers/mocks/vendorRisks/vendorRisks.data.ts (1)
Line range hint
1-71: Consider adding validation for vendor IDs in mock data.The mock data structure could benefit from runtime validation to ensure vendor IDs are consistent with the actual vendor records.
Consider adding a validation function:
interface VendorRisk { id: number; vendor_id: number; // ... other fields } function validateVendorRisks(risks: VendorRisk[]): void { const vendorIds = new Set(risks.map(risk => risk.vendor_id)); if (Math.max(...vendorIds) > risks.length) { console.warn('Warning: Some vendor IDs exceed the number of vendors'); } // Check for duplicate risk IDs const riskIds = risks.map(risk => risk.id); const duplicateRiskIds = riskIds.filter( (id, index) => riskIds.indexOf(id) !== index ); if (duplicateRiskIds.length > 0) { throw new Error(`Duplicate risk IDs found: ${duplicateRiskIds.join(', ')}`); } } // Usage: validateVendorRisks(vendorRisks);Servers/controllers/vendor.ctrl.ts (4)
Line range hint
87-94: Add validation for required fields.The validation only checks for
nameanddescription, but other fields liketype,status, andreview_statusmight also be required based on business logic.- if (!name || !description) { + const requiredFields = ['name', 'description', 'type', 'status']; + const missingFields = requiredFields.filter(field => !req.body[field]); + + if (missingFields.length > 0) { return res .status(400) - .json( - STATUS_CODE[400]({ message: "name and description are required" }) - ); + .json(STATUS_CODE[400]({ + message: `Missing required fields: ${missingFields.join(', ')}` + })); }
Line range hint
96-116: Add type validation for critical fields.Several fields need type validation to ensure data integrity:
status,review_status, andrisk_statusshould be validated against allowed valuesreview_dateshould be a valid date formatwebsiteshould be a valid URLConsider adding a validation helper:
interface ValidationRules { [key: string]: { type: string; values?: string[]; pattern?: RegExp; } } const VENDOR_VALIDATION_RULES: ValidationRules = { status: { type: 'enum', values: ['ACTIVE', 'INACTIVE', 'PENDING'] // adjust based on your requirements }, review_status: { type: 'enum', values: ['APPROVED', 'REJECTED', 'PENDING'] }, website: { type: 'string', pattern: /^https?:\/\/.+/ }, review_date: { type: 'date' } }; function validateVendorData(data: any): { isValid: boolean; errors: string[] } { const errors: string[] = []; Object.entries(VENDOR_VALIDATION_RULES).forEach(([field, rules]) => { if (data[field]) { if (rules.type === 'enum' && !rules.values?.includes(data[field])) { errors.push(`Invalid ${field}. Allowed values: ${rules.values?.join(', ')}`); } if (rules.type === 'date' && isNaN(Date.parse(data[field]))) { errors.push(`Invalid date format for ${field}`); } if (rules.pattern && !rules.pattern.test(data[field])) { errors.push(`Invalid format for ${field}`); } } }); return { isValid: errors.length === 0, errors }; }Also applies to: 118-138
Line range hint
141-190: Align updateVendorById with new vendor fields.The
updateVendorByIdfunction only allows updatingnameanddescription, whilecreateVendoraccepts many more fields. Consider updating this function to handle all updatable vendor fields.- const { name, description } = req.body; + const updatableFields = [ + 'name', 'description', 'type', 'website', 'contact_person', + 'assignee', 'status', 'review_result', 'reviewer', + 'review_date', 'review_status', 'risk_status' + ]; + const updateData = Object.fromEntries( + Object.entries(req.body).filter(([key]) => updatableFields.includes(key)) + ); - if (!name || !description) { + if (Object.keys(updateData).length === 0) { return res .status(400) - .json(STATUS_CODE[400]({ message: "name and description are required" })); + .json(STATUS_CODE[400]({ message: "No valid fields to update" })); }
Line range hint
139-140: Enhance error handling with specific error types.The current error handling catches all errors and returns a generic 500 status. Consider implementing more specific error handling for different scenarios.
class ValidationError extends Error { constructor(message: string) { super(message); this.name = 'ValidationError'; } } class DatabaseError extends Error { constructor(message: string) { super(message); this.name = 'DatabaseError'; } } // Example usage in catch block: catch (error) { if (error instanceof ValidationError) { return res.status(400).json(STATUS_CODE[400](error.message)); } if (error instanceof DatabaseError) { return res.status(503).json(STATUS_CODE[503](error.message)); } return res.status(500).json(STATUS_CODE[500]('An unexpected error occurred')); }Also applies to: 191-193
Servers/driver/autoDriver.driver.ts (1)
154-154: Improve value generation safety and handling.The current implementation could be improved to handle edge cases and prevent potential issues:
- Add type validation
- Handle null values
- Implement proper SQL escaping
Consider this safer implementation:
- return `('${vendor.name}', '${vendor.type}', '${vendor.description}', '${vendor.website}', '${vendor.contact_person}', '${vendor.assignee}', '${vendor.status}', '${vendor.review_result}', '${vendor.review_status}', '${vendor.reviewer}', '${vendor.review_date}', '${vendor.risk_status}')`; + const escape = (value: any) => { + if (value === null || value === undefined) return 'NULL'; + return `'${String(value).replace(/'/g, "''")}'`; + }; + return `(${escape(vendor.name)}, ${escape(vendor.type)}, ${escape(vendor.description)}, + ${escape(vendor.website)}, ${escape(vendor.contact_person)}, ${escape(vendor.assignee)}, + ${escape(vendor.status)}, ${escape(vendor.review_result)}, ${escape(vendor.review_status)}, + ${escape(vendor.reviewer)}, ${escape(vendor.review_date)}, ${escape(vendor.risk_status)})`;Servers/swagger.yaml (1)
2708-2749: Enhance the Vendor schema with audit trail fields and improved documentation.The schema should be enhanced with:
- Audit trail fields for tracking record changes
- More descriptive examples that showcase different vendor types and statuses
- Consistent date format
Apply these changes to the Vendor schema:
Vendor: type: object properties: id: type: integer example: 1 name: type: string - example: "Apex" + example: "Apex Technologies Inc." type: type: string - example: "Contractor" + example: "Technology Vendor" + description: "Type of vendor relationship (e.g., Technology Vendor, Service Provider, Contractor)" description: type: string - example: "Project: Chatbot AI, Services: NLP model development and maintenance" + example: "Primary AI/ML technology provider for our chatbot platform. Responsible for NLP model development, training, and maintenance." # ... other fields ... review_date: type: string - example: "12 January 2024" + format: date-time + example: "2024-01-12T00:00:00.000Z" + created_at: + type: string + format: date-time + example: "2024-01-01T00:00:00.000Z" + description: "Timestamp when the vendor record was created" + updated_at: + type: string + format: date-time + example: "2024-01-12T00:00:00.000Z" + description: "Timestamp when the vendor record was last updated"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Servers/controllers/vendor.ctrl.ts(1 hunks)Servers/driver/autoDriver.driver.ts(1 hunks)Servers/mocks/vendorRisks/vendorRisks.data.ts(2 hunks)Servers/swagger.yaml(2 hunks)
🔇 Additional comments (4)
Servers/mocks/vendorRisks/vendorRisks.data.ts (1)
46-46: Verify vendor ID references across the codebase.
The changes to vendor IDs (changing from 4→2 and 5→1) could lead to inconsistencies if these IDs are referenced elsewhere in the codebase.
Let's verify the vendor ID references:
Also applies to: 60-60
✅ Verification successful
No vendor ID inconsistencies found in mock data changes
The changes to vendor IDs in Servers/mocks/vendorRisks/vendorRisks.data.ts are safe as:
- Client-side mock data in
Clients/src/presentation/mocks/risks/risks.data.tsonly uses vendor IDs 1-3 - Vendor list in
Clients/src/presentation/mocks/vendors/vendors.data.tsonly defines vendors with IDs 1-3 - Project vendor risks data uses string-based vendor names instead of IDs
- No hardcoded references to vendor IDs 4 or 5 were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded references to vendor IDs 4 and 5
# that might need to be updated to match these changes
# Search for vendor ID references in TypeScript/JavaScript files
echo "Checking for vendor ID references..."
rg -t ts -t js "vendor.*[^0-9][45][^0-9]" --glob '!**/vendorRisks.data.ts'
# Search for vendor IDs in test files
echo "Checking test files..."
rg -t ts -t js "vendor.*id.*[^0-9][45][^0-9]" --glob '**/test/**'
# Search for vendor IDs in SQL migrations or schemas
echo "Checking database files..."
rg "vendor.*[^0-9][45][^0-9]" --glob '**/*.sql'
Length of output: 956
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Search for any numeric vendor IDs (to see all vendor ID patterns)
echo "Searching for vendor ID patterns..."
rg -t ts -t js "vendor.*id.*\d+" --glob '!**/vendorRisks.data.ts'
# 2. Look for test files specifically mentioning vendors
echo "Searching for vendor-related test files..."
fd -e ts -e js -e spec -e test vendor
# 3. Check for any mock data files that might reference vendors
echo "Searching for mock data files..."
fd -e ts -e js -g '*mock*' -g '*fixture*' -g '*test*data*'
# 4. Look for database schema or migration files
echo "Searching for database files..."
fd -e sql
Length of output: 1951
Script:
#!/bin/bash
# Let's check the content of the client-side mock data to ensure consistency
# Check the client-side risks mock data
echo "Content of client-side risks mock data:"
cat "Clients/src/presentation/mocks/risks/risks.data.ts"
# Check the vendors mock data
echo -e "\nContent of client-side vendors mock data:"
cat "Clients/src/presentation/mocks/vendors/vendors.data.ts"
# Check project vendor risks data
echo -e "\nContent of project vendor risks mock data:"
cat "Clients/src/presentation/mocks/projects/project-vendor-risks.data.ts"
Length of output: 8057
Servers/driver/autoDriver.driver.ts (2)
152-152: LGTM! Insert statement correctly matches table structure.
The insert statement properly includes all fields in the correct order.
139-154: Verify impacts of vendor table changes.
The significant changes to the vendor table structure might affect existing queries and type definitions.
Let's verify the impacts:
✅ Verification successful
Let's gather more specific information about the SQL queries and Vendor interface to verify if they align with the schema changes.
Schema changes are consistent with existing implementation
The verification shows that the vendor table schema changes align perfectly with:
- The Vendor interface in
Servers/models/Vendor.ts - SQL queries in
Servers/utils/vendor.utils.ts - Insert operations in
Servers/driver/autoDriver.driver.ts
All fields in the schema modification (name, type, description, website, contact_person, assignee, status, review_result, review_status, reviewer, review_date, risk_status) match exactly with:
- The interface definition
- The INSERT queries
- The field references in UPDATE/SELECT operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for SQL queries and type definitions that might need updates
# Test 1: Find SQL queries referencing the old vendor fields
echo "Checking SQL queries..."
rg -l "SELECT.*FROM\s+vendors|UPDATE\s+vendors|INSERT\s+INTO\s+vendors"
# Test 2: Find Vendor type/interface definition
echo "Checking Vendor type definition..."
rg -l "interface\s+Vendor|type\s+Vendor"
# Test 3: Find vendor-related migrations
echo "Checking migration files..."
fd -e sql -e ts "vendor|migration"
Length of output: 1206
Script:
#!/bin/bash
# Check the actual SQL queries and Vendor interface implementation
# Get SQL queries from the identified files
echo "=== SQL Queries in vendor.utils.ts ==="
rg "SELECT.*FROM\s+vendors|UPDATE\s+vendors|INSERT\s+INTO\s+vendors" Servers/utils/vendor.utils.ts -A 2
echo -e "\n=== Vendor Interface Definition ==="
rg "interface Vendor" Servers/models/Vendor.ts -A 15
echo -e "\n=== Other Vendor-related SQL operations ==="
rg "SELECT.*FROM\s+vendors|UPDATE\s+vendors|INSERT\s+INTO\s+vendors" Servers/driver/autoDriver.driver.ts -A 2
Length of output: 1914
Servers/swagger.yaml (1)
Line range hint 2198-2749: Overall implementation is well-structured and secure.
The vendor management API endpoints and schema are well-implemented with:
- Proper JWT authentication
- Consistent RESTful patterns
- Comprehensive error handling
- Clear documentation structure
The suggested improvements for field validation and schema documentation will further enhance the robustness and maintainability of the API.
| const { | ||
| name, | ||
| type, | ||
| description, | ||
| website, | ||
| contact_person, | ||
| assignee, | ||
| status, | ||
| review_result, | ||
| reviewer, | ||
| review_date, | ||
| review_status, | ||
| risk_status | ||
| } = req.body; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify variable access pattern.
The destructuring of request body fields is redundant since the values are later accessed directly via req.body. Either use the destructured variables consistently or remove the destructuring.
- const {
- name,
- type,
- description,
- website,
- contact_person,
- assignee,
- status,
- review_result,
- reviewer,
- review_date,
- review_status,
- risk_status
- } = req.body;
+ const vendorData = {
+ name: req.body.name,
+ description: req.body.description,
+ type: req.body.type,
+ website: req.body.website,
+ contact_person: req.body.contact_person,
+ assignee: req.body.assignee,
+ status: req.body.status,
+ review_result: req.body.review_result,
+ reviewer: req.body.reviewer,
+ review_date: req.body.review_date,
+ review_status: req.body.review_status,
+ risk_status: req.body.risk_status
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { | |
| name, | |
| type, | |
| description, | |
| website, | |
| contact_person, | |
| assignee, | |
| status, | |
| review_result, | |
| reviewer, | |
| review_date, | |
| review_status, | |
| risk_status | |
| } = req.body; | |
| const vendorData = { | |
| name: req.body.name, | |
| description: req.body.description, | |
| type: req.body.type, | |
| website: req.body.website, | |
| contact_person: req.body.contact_person, | |
| assignee: req.body.assignee, | |
| status: req.body.status, | |
| review_result: req.body.review_result, | |
| reviewer: req.body.reviewer, | |
| review_date: req.body.review_date, | |
| review_status: req.body.review_status, | |
| risk_status: req.body.risk_status | |
| }; |
| type varchar(100), | ||
| description text, | ||
| website varchar(255), | ||
| contact_person varchar(100), | ||
| assignee varchar(100), | ||
| status varchar(100), | ||
| review_result varchar(50), | ||
| review_status varchar(50), | ||
| reviewer_id integer, | ||
| review_date timestamp, | ||
| risk_status varchar(50), | ||
| CONSTRAINT vendors_reviewer_id_fkey FOREIGN KEY (reviewer_id) | ||
| REFERENCES users(id) | ||
| ON DELETE SET NULL, | ||
| CONSTRAINT vendors_project_id_fkey FOREIGN KEY (project_id) | ||
| REFERENCES projects(id) | ||
| ON DELETE SET NULL | ||
| reviewer varchar(50), | ||
| review_date varchar(50), | ||
| risk_status varchar(50) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schema design improvements needed for the vendors table.
Consider the following improvements to the table structure:
- Add NOT NULL constraints for critical fields (type, status)
- Add foreign key constraints for user references
- Use proper date type for review_date
- Add indexes for frequently queried fields
Apply these changes to improve data integrity and query performance:
- type varchar(100),
+ type varchar(100) NOT NULL,
description text,
website varchar(255),
contact_person varchar(100),
- assignee varchar(100),
+ assignee integer,
- status varchar(100),
+ status varchar(100) NOT NULL,
review_result varchar(50),
review_status varchar(50),
- reviewer varchar(50),
+ reviewer integer,
- review_date varchar(50),
+ review_date timestamp,
- risk_status varchar(50)
+ risk_status varchar(50),
+ CONSTRAINT fk_assignee FOREIGN KEY (assignee)
+ REFERENCES users(id)
+ ON DELETE SET NULL,
+ CONSTRAINT fk_reviewer FOREIGN KEY (reviewer)
+ REFERENCES users(id)
+ ON DELETE SET NULLCommittable suggestion skipped: line range outside the PR's diff.
| type: string | ||
| example: "Active" | ||
| review_result: | ||
| type: string | ||
| example: "Positive, needs minor improvements" | ||
| reviewer: | ||
| type: string | ||
| example: "George Michael" | ||
| review_date: | ||
| type: string | ||
| example: "12 January 2024" | ||
| review_status: | ||
| type: string | ||
| example: "Under Review" | ||
| risk_status: | ||
| type: string | ||
| example: "High" | ||
| required: true | ||
| responses: | ||
| "202": | ||
| description: vendor | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: accepted | ||
| data: | ||
| $ref: "#components/schemas/Vendor" | ||
| "404": | ||
| description: no content to display | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: not found | ||
| data: | ||
| type: object | ||
| "500": | ||
| description: internal server error | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: internal server error | ||
| error: | ||
| type: object | ||
| delete: | ||
| tags: [vendors] | ||
| security: | ||
| - JWTAuth: [] | ||
| parameters: | ||
| - in: path | ||
| name: id | ||
| schema: | ||
| type: integer | ||
| required: true | ||
| description: id of the vendor to delete | ||
| responses: | ||
| "202": | ||
| description: vendor | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: accepted | ||
| data: | ||
| type: boolean | ||
| example: true | ||
| "404": | ||
| description: no content to display | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: not found | ||
| data: | ||
| type: object | ||
| "500": | ||
| description: internal server error | ||
| content: | ||
| application/json: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| example: internal server error | ||
| error: | ||
| type: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validation constraints and standardize date format.
The vendor endpoints are well-structured but could benefit from the following improvements:
- Add validation constraints for:
websitefield to ensure valid URLsstatus,review_status, andrisk_statusfields to restrict to valid enum values
- Standardize the date format:
- Use
format: date-timefor thereview_datefield to maintain consistency with other date fields in the API
- Use
Apply these changes to both POST and PUT endpoint request bodies:
website:
type: string
example: "https://apex-ai.com"
+ format: uri
status:
type: string
example: "Active"
+ enum: ["Active", "Inactive", "Suspended"]
review_status:
type: string
example: "Under Review"
+ enum: ["Not Started", "Under Review", "Completed"]
risk_status:
type: string
example: "High"
+ enum: ["Low", "Medium", "High"]
review_date:
type: string
example: "12 January 2024"
+ format: date-time
+ example: "2024-01-12T00:00:00.000Z"Committable suggestion skipped: line range outside the PR's diff.
|
@HarshP4585 |
Affected Issue: #155
Summary by CodeRabbit
Release Notes
New Features
/vendorsendpoint for managing vendor records, including methods for listing, creating, updating, and deleting vendors.Bug Fixes
Documentation