Improvements for non-zero index subtotals #6
Conversation
- the `collection` is empty and you set `collection[1] = {}`, then `collection[0]` will be `nil` and that'll cause things to blow up
- eg, when the subtotal index is 1, you'll have an index_1 row, but you don't want an empty index_0 row
|
Taking a look at this PR as I have several client requests that will benefit from this being merged in. Related Client Requests |
There was a problem hiding this comment.
Looks good to me. One thing I noticed was that nonzero starting subtotals do not get the border top styling, see image below. I don't think that was intentional and if it wasn't I can add a to the PR here.
Only important if you're using the default styling but not important if you already override the css property (.index_0 td) in your code...I think.
Compared to a subtotal indexed at 0 below
| @@ -72,6 +72,48 @@ | |||
| expect(data_table.subtotal_calculations).to eq({["Star Wars"]=>[{:power_level=>{:sum=>145.0}}], ["Middle Earth"]=>[{:power_level=>{:sum=>9081.0}}]}) | |||
| end | |||
|
|
|||
| it "should do sub-totaling starting with indexes > 0" do | |||
| data_table.group_by :world, level: 0 | |||
There was a problem hiding this comment.
{level: 0} is the default value for the level param in the group by definition so it can be removed.
| data_table.group_by :world, level: 0 | |
| data_table.group_by :world |
There was a problem hiding this comment.
I think it should have the border top by default in that case. Let's go ahead and add that.
|
Here's my proposed changed for the problem mentioned above regarding the borders: #7 Thoughts on the implementation? Edit - closed pr since I need to look into it more. |
|
I think that using a css pseudo class here is a decent way to avoid the brittleness of relying on the indexes values generated by data-tables. Let's test using if that works then I'm ok with merging this change. |
| @@ -339,7 +339,9 @@ def calculate_subtotals! | |||
| @collection.each_pair_with_parents(@groupings.count) do |group_name, group_data, parents| | |||
| path = parents + [group_name] | |||
| result = calculate(group_data, subtotal[0], subtotal[1], path) | |||
| @subtotal_calculations[path][index] ||= {} | |||
| (0..index).each do |index| | |||
There was a problem hiding this comment.
@aharpervc What the reasoning for iterating over the range here? (0..index)
There was a problem hiding this comment.
Same logic as https://github.com/veracross/data-table/pull/6/files#diff-6d5c93afbede304616a4170b9f9dacadR443, which is that you need to fill in any prior array indexes with empty objects to avoid nil reference exceptions.
x = []
x[1] = {}
puts x.inspect # [nil, {}]`There was a problem hiding this comment.
Were nil references the original problem that triggered this change?
There was a problem hiding this comment.
Yep, see https://support.veracross.com/a/tickets/17520 for the original test case, now followed by all of brian's examples, plus https://github.com/axiom-dev/planning/issues/1274
|
Alright, I created this PR(#8) which keeps the top borders for nonzero subtotals and totals when they are the first rendered row. |
|
@aharpervc - Same question, different PR. This PR is ready to be merged into master. Do you have any objections? What needs to happen once this is merged to get it into production? |
The code seems fine in and of itself. The next question is integration with consumers & confirmation that it resolves the problem statement. Confirming that & getting to production are a similar path... get a dev env of an app you want to test with, change the gemfile to point at this branch, try to pull up the pages that formerly failed & are now expected to succeed (don't forget to confirm they fail on master branch first). Assuming that all works out, the production update steps are: follow the release steps for this gem (release notes, version bump, push to rubygems), then follow the release steps for any app that needs this update. |
|
Awesome, thanks. I'll work on testing inside a development environment. |
Now to go through the production steps. |
aharpervc
deleted the
fix-unhandled-exception-for-nonzero-starting-subtotals
branch
@ronnietaylor I guess you were the last person to touch this, so GitHub said you could review it 😀