Skip to content

Conversation

@mw3155
Copy link
Contributor

@mw3155 mw3155 commented Nov 24, 2024

User description

Fixes #3

Add validation to prevent setting negative stock values in CatalogItem model, CatalogController, and CatalogService.

  • CatalogItem Model:

    • Add validation attribute to AvailableStock property to ensure it cannot be set to negative values.
  • CatalogController:

    • Add validation in Create action to check for negative stock values and add error message to the frontend.
    • Add validation in Edit action to check for negative stock values and add error message to the frontend.
  • CatalogService:

    • Add validation in CreateCatalogItem method to check for negative stock values and throw an exception if the value is negative.
    • Add validation in UpdateCatalogItem method to check for negative stock values and throw an exception if the value is negative.
  • Views:

    • Add error message display for negative stock values in Create.cshtml.
    • Add error message display for negative stock values in Edit.cshtml.

For more details, open the Copilot Workspace session.


PR Type

Bug fix, Enhancement


Description

  • Added validation to CatalogController to prevent setting negative stock values in both Create and Edit actions.
  • Enhanced CatalogItem model with range validation to ensure AvailableStock cannot be negative.
  • Updated CatalogService to throw exceptions for negative stock values in CreateCatalogItem and UpdateCatalogItem methods.
  • Improved user feedback by adding validation message displays for negative stock values in Create.cshtml and Edit.cshtml views.

Changes walkthrough 📝

Relevant files
Bug fix
CatalogController.cs
Add validation for negative stock values in CatalogController

eShopOnWeb/eShop.MVC/Controllers/CatalogController.cs

  • Added validation to prevent negative stock values in Create action.
  • Added validation to prevent negative stock values in Edit action.
  • +8/-0     
    CatalogService.cs
    Add exception handling for negative stock in CatalogService

    eShopOnWeb/eShop.MVC/Services/CatalogService.cs

  • Added exception handling for negative stock values in
    CreateCatalogItem.
  • Added exception handling for negative stock values in
    UpdateCatalogItem.
  • +9/-1     
    Enhancement
    CatalogItem.cs
    Add range validation to AvailableStock in CatalogItem model

    eShopOnWeb/eShop.MVC/Models/CatalogItem.cs

  • Added range validation to AvailableStock to prevent negative values.
  • +2/-1     
    Create.cshtml
    Display validation message for negative stock in Create view

    eShopOnWeb/eShop.MVC/Views/Catalog/Create.cshtml

    • Added validation message display for negative stock values.
    +1/-0     
    Edit.cshtml
    Display validation message for negative stock in Edit view

    eShopOnWeb/eShop.MVC/Views/Catalog/Edit.cshtml

    • Added validation message display for negative stock values.
    +1/-0     

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

    Fixes #3
    
    Add validation to prevent setting negative stock values in `CatalogItem` model, `CatalogController`, and `CatalogService`.
    
    * **CatalogItem Model:**
      - Add validation attribute to `AvailableStock` property to ensure it cannot be set to negative values.
    
    * **CatalogController:**
      - Add validation in `Create` action to check for negative stock values and add error message to the frontend.
      - Add validation in `Edit` action to check for negative stock values and add error message to the frontend.
    
    * **CatalogService:**
      - Add validation in `CreateCatalogItem` method to check for negative stock values and throw an exception if the value is negative.
      - Add validation in `UpdateCatalogItem` method to check for negative stock values and throw an exception if the value is negative.
    
    * **Views:**
      - Add error message display for negative stock values in `Create.cshtml`.
      - Add error message display for negative stock values in `Edit.cshtml`.
    
    ---
    
    For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/GoatSwitch/dotnet-demo-projects/issues/3?shareId=XXXX-XXXX-XXXX-XXXX).
    @qodo-code-review
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    3 - Fully compliant

    Compliant requirements:

    • Added validation to prevent negative stock values
    • Added error messages for negative stock input
    • Implemented validation at model, controller, and service levels
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Redundant Validation
    Service-level validation might be redundant since model-level validation is already in place. Consider removing service validation to follow single responsibility principle.

    Redundant Validation
    Controller-level validation might be redundant since model-level validation is already in place. The ModelState.IsValid check would catch the Range validation error.

    @qodo-code-review
    Copy link

    qodo-code-review bot commented Nov 24, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    ✅ Remove duplicate validation logic to maintain single source of truth for business rules

    Remove redundant validation in service layer since validation is already handled by
    model attributes and controller. This avoids duplicate validation logic and
    potential inconsistencies.

    eShopOnWeb/eShop.MVC/Services/CatalogService.cs [53-56]

    -if (catalogItem.AvailableStock < 0)
    -{
    -    throw new ArgumentException("Stock value cannot be negative.");
    -}
    +// Remove validation check as it's handled by model validation

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: Valid suggestion to eliminate redundant validation logic, as the validation is already handled at the model and controller levels. This improves code maintainability and reduces the risk of inconsistent validation rules.

    8
    Remove redundant manual validation in favor of attribute-based validation

    Remove manual ModelState validation since the Range attribute on AvailableStock
    property will automatically handle this validation.

    eShopOnWeb/eShop.MVC/Controllers/CatalogController.cs [66-69]

    -if (catalogItem.AvailableStock < 0)
    -{
    -    ModelState.AddModelError("AvailableStock", "Stock value cannot be negative.");
    -}
    +// Remove manual validation as it's handled by model attributes
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Excellent suggestion to remove duplicate validation since the Range attribute on the model will automatically handle this validation. This simplifies the code and maintains validation logic in a single place.

    8
    Use appropriate numeric type to enforce non-negative constraint at type level

    Consider using unsigned integer (uint) instead of int with Range validation to
    inherently prevent negative values at the type level.

    eShopOnWeb/eShop.MVC/Models/CatalogItem.cs [46-47]

    -[Range(0, int.MaxValue, ErrorMessage = "Stock value cannot be negative.")]
    -public int AvailableStock { get; set; }
    +public uint AvailableStock { get; set; }
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While using uint would enforce non-negative values at the type level, it might introduce compatibility issues with existing code and database schema. The current Range validation approach is sufficient and more flexible.

    4

    💡 Need additional feedback ? start a PR chat

    Comment on lines +53 to +56
    if (catalogItem.AvailableStock < 0)
    {
    throw new ArgumentException("Stock value cannot be negative.");
    }

    Choose a reason for hiding this comment

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

    Suggestion: Remove duplicate validation logic to maintain single source of truth for business rules [General, importance: 8]

    Suggested change
    if (catalogItem.AvailableStock < 0)
    {
    throw new ArgumentException("Stock value cannot be negative.");
    }
    // Remove validation check as it's handled by model validation

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    bug: user should not be able to set stock to negative numbers

    2 participants