Implement corporate pricing into invoice items #546
Conversation
There was a problem hiding this comment.
This is great! This is a great start, and a perfectly valid way to do this.
I do want to think about one other way of doing this: Over the many years of Tech, there have been quite a few different pricing schemes: student org, corporate, AB, personal, class, rental, etc. On top of that, pricing and these structures change every year. Possibly instead of adding one field to invoice items, what if we had invoice list item groups? This would be a lot more flexible. It could look like this:
- Groups like FY23-24 Default, FY23-24 Corporate, FY23-24 Rental
- A new set of groups for each year makes it much easier to manage/keep up with in Tracker (can make a duplicate button) and understand past pricing later
- Allows for different groups structure every year
- Could make groups ordered (so default appears at the top, then corporate)
- Could have a dropdown at the top of the invoice to select the group (in JavaScript) or use select box headings
Let me know what you think!
| @@ -217,7 +217,8 @@ | |||
| create_table "invoice_items", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| | |||
| t.string "memo", limit: 255, null: false | |||
| t.string "category", limit: 255, null: false | |||
| t.integer "price", null: false | |||
There was a problem hiding this comment.
This should be big decimal. Floats are evil. The invoice price field must be float because big decimal didn't exist yet; we should fix that at some point.
Since this is just the line items this is not a massive migration (at first I thought it was also the invoice table).
| @@ -0,0 +1,4 @@ | |||
| class ChangeInvoiceItemPriceToFloat < ActiveRecord::Migration[6.1] | |||
There was a problem hiding this comment.
This is missing the float migration and new column.
| <% if @invoice.payment_type == "Oracle" %> | ||
| <option data-memo="<%= item.memo %>" data-category="<%= item.category %>" data-price="<%= item.corporate %>"><%= item.memo %></option> | ||
| <% else %> | ||
| <option data-memo="<%= item.memo %>" data-category="<%= item.category %>" data-price="<%= item.price %>"><%= item.memo %></option> | ||
| <% end %> |
There was a problem hiding this comment.
I'm not certain if this is the best way to detect corporate pricing. In a lot of cases you won't have an Oracle string at first, and there have been cases where you have an Oracle string but it goes to a student org. I don't think we want to cause people to input incorrect data/omit data simply to switch pricing.
It would probably be better to have a separate checkbox implemented only in JavaScript (that switches the select boxes). However, I think you can do this a slightly different way. See other comments.
| @@ -45,6 +45,6 @@ def destroy | |||
|
|
|||
| private | |||
| def ii_params | |||
| params.require(:invoice_item).permit(:memo, :category, :price, :line_no) | |||
| params.require(:invoice_item).permit(:memo, :category, :price, :corporate, :line_no) | |||
There was a problem hiding this comment.
I would rename this to corporate_price to be more specific.
|
The way I envision using invoice items is simply as a shortcut when your lazy, where you're already famliiar with the pricing sheet and line items, and you just want to be able to enter a line item quicker. I think the pricing sheet is still what I would use if I'm doing a rental or am unsure of which line item to use, so adding groups to tracker would make the whole thing more cumbersome for not much gain. I do agree that the oracle string is a poor proxy, and a checkbox would be better, but I'd have to look through how to do that. |
Yeah, that's exactly what the invoice items feature is for and I think this still helps. You would have a dropdown at the top where you select "corporate" (maybe instead of a "corporate" checkbox) and then all the later dropdowns show the corporate line items just as you would have with the current state of the pull.
I guess my point is to keep Tracker flexible instead of hardcoding in one type of scheme where that may change or where there may be additional schemes sometime in the future. This sort of thing absolutely comes back to bite later--you want to avoid using using specific naming or columns where things should be groups. For example: the SLICE finance person's email was hardcoded in and we had to change code every time they changed advisors (once a semester for like 3 years). Eventually I added a model for a list of invoice recipients which also allowed for multiple SLICE people to receive them--which was not possible with the single-person hardcoding. Another example: Notice that there is not a column for each event role position on each event date; they are stored as text in a separate joined table. New roles aren't added very frequently, but when they are it we don't have to think twice about adding to every view, form, param list, validation, permission, etc. No migrations need to be run. The pricing sheet method came to exist because Tracker wasn't covering all of the different pricing structures anymore and nobody wanted to deal with changing Tracker at the time (not vice versa). This is also how the equipment system on Tracker has become unused. |
|
I guess my main concern is that a tracker feature that's as adaptable and versatile as a shared google sheets would take so much effort that it's not really worth it. I think hardcoding default and corporate works pricing works because (from my understanding) those are the two most basic pricing formats, and everything with a normal price has a corporate price that scales in the same way (by hour). Other pricing modes are rare or complicated enough that I think people will just go to the pricing sheet. |
Adds corporate price to invoice items. Invoice line price will automatically choose corporate pricing iff the invoice has the oracle option. Also changes invoice item prices to floats to align with invoice line prices being floats.
Note that I was unable to test this locally due to JavaScript issues.