Skip to content

Conversation

1robroos
Copy link

@1robroos 1robroos commented Oct 5, 2025

Summary

This PR adds comprehensive Purchase Orders functionality to Snipe-IT, enabling organizations to track asset procurement from order creation through delivery and asset assignment.

Features Added

  • Complete CRUD operations for Purchase Orders and Purchase Order Items
  • PDF generation with company branding and professional formatting
  • Asset linking with purchase order tracking and lookup functionality
  • Enhanced navigation with Purchase Orders menu in both main nav and settings
  • Database migrations for purchase_orders, purchase_order_items, and asset linking
  • Comprehensive documentation with installation guide and screenshots
  • API endpoints for programmatic access
  • Status management (Draft, Pending Approval, Ordered, Received, Cancelled)
  • Supplier integration and asset model compatibility
  • Backwards compatible implementation with existing Snipe-IT functionality

Documentation

See PURCHASE_ORDERS_README.md for complete installation guide, feature overview, and screenshots.

Testing

  • Tested on fresh Snipe-IT installation
  • All database migrations run successfully
  • Menu integration works correctly
  • PDF generation functional
  • Asset linking operational

Breaking Changes

None - fully backwards compatible with existing installations.

- Complete CRUD operations for Purchase Orders and Purchase Order Items
- PDF generation with company branding and professional formatting
- Asset linking with purchase order tracking and lookup functionality
- Enhanced navigation with Purchase Orders menu in both main nav and settings
- Database migrations for purchase_orders, purchase_order_items, and asset linking
- Comprehensive documentation with installation guide and screenshots
- API endpoints for programmatic access
- Status management (Draft, Pending Approval, Ordered, Received, Cancelled)
- Supplier integration and asset model compatibility
- Backwards compatible implementation with existing Snipe-IT functionality
@1robroos 1robroos requested a review from snipe as a code owner October 5, 2025 09:07
@snipe
Copy link
Member

snipe commented Oct 5, 2025

Hi @1robroos - thanks for this! It’s going to take a bit of time to review, as it’s pretty big, but I had a few initial questions/comments:

  1. Why is there a BIN directory and readme in this?
  2. It looks like this only relates to assets, not other types of items, is that correct?
  3. Are we trying to pull in any existing purchase orders/order numbers?
  4. I’m seeing a lot of partials here where we wouldn’t normally use them - is there a reason for that?
  5. Definitely will need those language strings translated. (We use en-US, not en here)
  6. We removed the HTML -> PDF package we were previously using, so I’m not sure if your PDF still will still work as expected (I have to look closer) - but if it does, we should be using the print logo versus the regular logo there and should probably allow for the css_coverrides to be injected
  7. I noticed a few unescaped places where we echo the request back out onto the screen - those should probably be fixed
  8. I also noticed a few data- attributes that seem to have been removed from the assets listing page table that should probably be put back
  9. When describing Eloquent relationships, we usually just describe the relation, versus getBlahBlahBlah, as it helps us understand the purpose of the method when looking at the places where it’s implemented.

@1robroos
Copy link
Author

1robroos commented Oct 5, 2025

Hello @snipe ,

Thank you for the detailed feedback! I really appreciate the thorough review. Let me address each point:

  1. BIN directory and readme: I'm not seeing a BIN directory in my commit or local files. Could you clarify which specific BIN directory you're referring to? The PURCHASE_ORDERS_README.md was added as feature documentation - happy to relocate if needed.

  2. Assets only: Correct - this implementation is specifically designed for assets. The company that I was trying to help with this is mainly using assets, so I narrowed the scope. While the basic purchase order structure could theoretically be extended to other item types, the current implementation has asset-specific fields and logic throughout.

  3. Existing purchase orders: Currently it only creates new POs with auto-generated numbers in the format PO-YYYY-#### (e.g., PO-2025-0001). There'sno import functionality for existing purchase order data.

  4. Partials usage: As mentioned on Discord, I've been working with Amazon Q Developer as an AI coding assistant for this implementation. Some of the architectural choices (like the partials structure) may reflect that collaborative approach rather than pure Snipe-IT conventions.

  5. Language strings: Understood - will need en-US structure and proper trans() calls.

  6. PDF package: Thanks for the heads up about the PDF package changes. I wasn't aware of the recent removal of the HTML->PDF package. The current implementation may need updating to work with whatever PDF solution Snipe-IT now uses.

  7. Unescaped output: I've reviewed the code with Amazon Q Developer and we're not immediately seeing where this occurs - the {!! $errors->first() !!} patterns used match the existing Snipe-IT error handling approach. Could you point out the specific locations you're concerned about so we can understand what needs to be addressed?

  8. Missing data- attributes: You're right - I see that data-show-columns-search="true" and data-buttons="assetButtons" were removed from the assets listing table during the integration. These should be restored to maintain the existing table functionality. I also added some new data attributes (data-show-export, data-show-refresh) - should those be kept or removed?

  9. Eloquent relationships: Looking at the code with Amazon Q Developer, the relationship methods appear to follow the standard naming pattern (supplier(), items(), creator()) rather than getBlahBlahBlah. Could you point out specific methods you'd like renamed so I understand which ones need adjustment?

This was quite an undertaking (31 files, 5800+ lines) and I appreciate the guidance on project standards. As mentioned on Discord, I worked primarily as a project director while Amazon Q Developer handled most of the actual coding implementation. Given the scope of refinements needed and my learning curve with Snipe-IT conventions, would the team be open to collaborative improvements on specific high-priority items?

@marcusmoore
Copy link
Collaborator

Hi @1robroos and thanks for taking a swing at adding purchase orders to the application!

Introducing the concept of purchase orders is a big undertaking and something we would like to include in the application. We've had a few PRs for this already, including some internal work, but they weren't merged due to not being the approach we want to introduce and maintain going forward.

I've only given the code in this PR a glance but I can see that it doesn't follow the conventions of the rest of our application. You mentioned using Amazon Q Developer. Unfortunately, in it's current state, AI tools don't take the entire application into context as it generates changes. The code in the PR is code that runs (I assume) but looks a lot different than the rest of our codebase. Ultimately, the code and workflows that are merged need to be maintained and supported by our team in the future so we strive to avoid introducing code that is "unexpected".

Honestly, there are a lot of changes that would need to be made before this PR is in a testable and mergable state and it'll be an undertaking to list all of them. If you were to use this as a base to roll your sleeves up and make the needed changes manually we can begin to add inline comments to the PR review outlining what needs to be updated. In my experience, prompting AI agents for changes requires being extremely specific and doesn't take into account implied meanings that occur when human developers communicate with the context of the codebase in their brain. AI also tends to go beyond what changes you're asking it for (for example, I noticed a lot of unneeded/unrelated changes in the AssetPresenter that makes reviewing that file difficult).

I don't mean to dismiss your contribution but want to let you know the reality of what would need to happen for this PR to be merged.

@1robroos
Copy link
Author

Hi @marcusmoore,

Thank you for the honest assessment. I appreciate the time you took to review this.

Given the scope of changes needed to align with Snipe-IT conventions and my limitations as a non-professional developer, I don't think I can realistically deliver the quality of refactoring this PR would require.
I hope the work here might still provide some value as reference material for future purchase order implementations by your team or other contributors who have deeper familiarity with the codebase patterns.

Thanks for considering the contribution and for the constructive feedback.

@marcusmoore marcusmoore linked an issue Oct 14, 2025 that may be closed by this pull request
@marcusmoore
Copy link
Collaborator

Thank you again for your contribution. I'm going to close this PR but I've attached it to the relevant issue so it can be used as reference in the future.

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.

Feature request: Invoice/Order Tracking

3 participants