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

Added threshold notifications and min qty for Asset models #13526

Merged
merged 13 commits into from
Sep 11, 2023

Conversation

Godmartinz
Copy link
Collaborator

Description

This adds a minimum quantity threshold for Asset Models. Also updates the notifcations to include Asset Models.

image image

Fixes #sc-19826

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • PHP version:
  • MySQL version
  • Webserver version
  • OS version

Checklist:

@what-the-diff
Copy link

what-the-diff bot commented Aug 24, 2023

PR Summary

  • Inclusion of New 'Asset' and 'AssetModel' Models
    Our system has been equipped with new models, specifically 'Asset' and 'AssetModel'. These were added to the Helper file, enhancing our app system's capabilities.

  • Addition of 'min_amt' Field to API Response
    There's a new field 'min_amt' included in the API response, contributing to the data it provides, making it more informative.

  • Inclusion of 'min_amt' Field in the 'AssetModel'
    The 'AssetModel' is now updated with the new 'min_amt' field.

  • Insertion of 'min_amt' Field in the Transformer
    An addition to our Transformer is the 'min_amt' field, improving the functionality of the system's data transformation process.

  • Migration used to Add 'min_amt' Column to the Models Table
    A new column 'min_amt' has been added to the 'models' table with the help of a migration adding a new level of details to the data set.

  • Minimum Quantity Input Field Added to the Asset Model Edit View
    A new input field meant for minimum quantity has been introduced to the asset model edit view, providing a convenient option to input the minimum quantity directly from the view itself.

Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

I think we also need to add min_amt to the fillable array in the AssetModel model since we use $assetmodel->fill($request->all()); in the API, otherwise additions/updates via the API won't be able to access that value. (Unless it's already in there and I just missed it.)

Otherwise this looks pretty solid.

Not for this PR, but we should also see about adding models to the inventory alert emails.

@Godmartinz
Copy link
Collaborator Author

min_amt is already added to fillable in the AssetModel model. Also, for the Inventory Alert email, where would the asset_model quantity info need to be sent? My understanding is that it goes to the Helper:checkLowInventory()

@snipe
Copy link
Owner

snipe commented Aug 30, 2023

Ah, you're right, my bad. The GH diff made it look weird. If you can yank those commented lines out, this looks good to go.

@snipe
Copy link
Owner

snipe commented Sep 11, 2023

Looks like we have a merge conflict now

@snipe
Copy link
Owner

snipe commented Sep 11, 2023

Nice, quick as a bunny :)

@Godmartinz
Copy link
Collaborator Author

Also, running the low inventory command recognizes and send out an email for asset models.
image

@snipe
Copy link
Owner

snipe commented Sep 11, 2023

The chipper test is failing on this, but I think it's related to an earlier merge.

@snipe snipe merged commit a49d3fe into snipe:develop Sep 11, 2023
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.

2 participants