(2931) Actual, Refund and Comment csv import row #2281

mec · 2023-12-13T11:49:13Z

Add Actial and Refund csv import row
WIP: Use new csv import row

Changes in this PR

Screenshots of UI changes

Before

After

Next steps

Is an ADR required? An ADR should be added if this PR introduces a change to the architecture.
Is a changelog entry required? An entry should always be made in CHANGELOG.md, unless this PR is a small tweak which has no impact outside the development team.
Do any environment variables need amending or adding?
Have any changes to the XML been checked with the IATI validator? See XML Validation

spec/support/shared_examples/behaves_like_financial_value.rb

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

CristinaRO · 2023-12-13T13:09:15Z

app/models/import/transactions/actual_and_refund_csv_row.rb

+    valid
+  end
+
+  private def blank?(value)


I find the naming of this method confusing, because it's too close to the Rails method blank?.

Interesting, I went with blank as that is the language used in the existing implementation for 'empty' when informing the user:

beis-report-official-development-assistance/app/services/actual/import.rb

Line 183 in 0378e81

def validate_values

I think I preferred the idea of 'empty' but that is also a Rails method!

It was original nil, but this became tricky and messy in parts, in the end I decided to stick to the path that was already trodden.

Happy to do something else though.

CristinaRO · 2023-12-13T13:12:47Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+        allow(csv_row).to receive(:field).with("Activity RODA Identifier").and_return(nil)
+        row = described_class.new(csv_row)
+
+        expect(row.roda_identifier).to eql(:blank)


It took me two readings to realise you were creating a symbol named :blank and not using the built-in Rails blank?. 😅

Could you either use nil, or empty string, or name the symbol something that doesn't clash with Rails methods?

See above, nil was my first choice but got tricky and I guess nil doesn't feel quite right here?

The side effect of returning :blank is the user is shown that the cell that has the error is 'blank' and not '' (empty string) which is the existing behaviour (I think?) see:

beis-report-official-development-assistance/app/services/actual/import.rb

Line 183 in 0378e81

def validate_values

CristinaRO · 2023-12-13T13:13:52Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+      expect(row.comment).to eql("This is a comment.")
+    end
+
+    context "when the comment is empty" do


What happens if the comment is an empty string instead of nil?

My understanding here is that CSV::Row, which is the expected passed in object returns nil for empty values, you would have to do something else to get an empty string?

CristinaRO · 2023-12-13T13:18:11Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+  end
+
+  describe "#transaction_type" do
+    context "when the actual value is not zero and the refund value is zero" do


What does it return when the actual value is not zero and the refund value is an empty string or nil?

I would expect it to return :actual.

actual_value cannot return an empty string or nil, you will always get something or :blank by design.

" " (space) on the other hand is a problem that needs to handled as well!?

CristinaRO · 2023-12-13T13:19:33Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+      end
+    end
+
+    context "when the actual value is zero and the refund value is not zero" do


It should also return :refund if the actual value is an empty string or nil, just in case the user has deleted the pre-populated zero.

refund_value cannot be an empty string or nil by design - we try to represent that as :blank right now.

See note about about " " (space) though.

CristinaRO · 2023-12-13T13:20:23Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+      it "returns refund" do
+        csv_row = double(CSV::Row)
+        allow(csv_row).to receive(:field).with("Actual Value").and_return("0")
+        allow(csv_row).to receive(:field).with("Refund Value").and_return("10000.32")


If the refund value is not numeric, does it return :refund and let the validation deal with the non-numeric elsewhere?

Certainly the validation and the values are intentionally treated seperately in most cases - this is so the invalid values can be shown to the user in the error messages.

I theory transaction_type is used later when we are creating the models and this would only happen if the row was valid, see:

beis-report-official-development-assistance/app/services/actual/import.rb

Line 75 in 22cdbee

if row.valid?

But I admit it's less than ideal and I need to guard here for invalid rows and return nil in that case - but that leads to the question, what do we do then?

I've added a guard that represents:

we cannot infer the type unless the actual and refund value are numeric

which would equate to an invalid row anyway that would not be imported - but now we cover the case be returning nil

It might be better to move this responsibility out of this class and let the calling object decide what to do based on the values and validity of the row - let me have a look.

Wow, moving the responsibility up creates a bunch of duplication I am not super happy with! I'll leave it witht the guard for now.

CristinaRO · 2023-12-13T13:22:30Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+      end
+    end
+
+    context "when the actual value is zero and the refund value is zero" do


What happens when the actual value is zero and the refund value is an empty string or nil, and there is a comment? I would expect it to also be :comment.

Or when the actual value and refund value are both empty strings/nil, and there is a comment?

The first state cannot happen as refund_value returns a string or :blank along with " " (space)!

The second state is explicitally considered invalid at the moment by desgin, I made the decision to expect explicit zeros when adding a 'activity comment' - we can change that decision, I just need to know and Ideally, know all the states first.

I documented all this before we started on this Miro, it might be worth going over it and updating with what we thing the states should be?

https://miro.com/app/board/uXjVNItge2M=/?share_link_id=923415912018

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

CristinaRO · 2023-12-13T13:27:20Z

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

+            row = described_class.new(csv_row)
+
+            expect(row).to be_invalid
+            expect(row.errors["Comment"][1]).to include("must both be zero to provide a comment")


Ah, this is an assumption that I would like to validate with Dan & co.

For the PO users' convenience, I would expect that they would prefer to not have to add a zero in the Refund value column manually. Is there a strong need for ease of development to require this?

I went with the explicit '0' to try and make it clear the intention is to say:

I have 0 actual value and 0 refund value and a comment to leave

But I don't mind what we do, it just needs deciding?

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb

We want to offer users the ability to report: - 0 actual - 0 refund - provide a comment as to the zeros The existing 'actuals importer' handles both Actual and Refund Transactions and has been extended over time. The import process is quite complex as there are dependencies across the columns. Rather than continue to extend the existing class, we decided to create a new one that will make the code more approachable and support the new behaviour. As the existing class is broken down over three loose classes with responsibilities to: - convert the supplied csv data to useable values - validate the values - authorise the activities - create the actual objects The responsibility is shared across the classes and can be hard to follow. We decided that the shape of this problem has a lot of similarities to accepting user supplied values from a web form and in those instances, we will often reach for the 'form object' pattern, a standin for the object we wish to create that can be validated seperately and then used to create the model or models. We felt this is pretty much the same thing here except the input is not a set of strings submitted via a form, rahter strings from a CSV file. The new `Import::Transactions::ActualsAndRefundCsvRow` class does just this, for a single row in the CSV file: - it returns any value from the csv that is required to import the modelled row - it explicitally treats an 'empty' value as a 'blank' - it validates the values seperately - it retains the original values even when invalid to assit the user later - it loosely behaves like an ActiveRecord model The downside is that we have to duplicate all the validation from the underlying models, but that price feels worth paying to make the code more approachable - the result of calling `valid?` is an object than can be used to create the underlying objects in the database or not, with errors that explain why (just like feedback on a form!). By loosely maintain the object interface, refactoring the actual importing process is relatively simple.

mec · 2023-12-20T14:35:23Z

Thanks for all the feedback, this work has been superceded by #2284, closing now.

CristinaRO reviewed Dec 13, 2023

View reviewed changes

spec/support/shared_examples/behaves_like_financial_value.rb Outdated Show resolved Hide resolved

CristinaRO reviewed Dec 13, 2023

View reviewed changes

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb Outdated Show resolved Hide resolved

CristinaRO reviewed Dec 13, 2023

View reviewed changes

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb Show resolved Hide resolved

CristinaRO reviewed Dec 13, 2023

View reviewed changes

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb Outdated Show resolved Hide resolved

CristinaRO reviewed Dec 13, 2023

View reviewed changes

spec/models/import/transactions/actual_and_refund_csv_row_spec.rb Outdated Show resolved Hide resolved

mec force-pushed the feature/2931-actual-refund-comment-csv-imoprt-row branch 2 times, most recently from 3bd72b7 to 0556124 Compare December 13, 2023 15:42

mec added 2 commits December 13, 2023 16:03

WIP: Use new csv import row

a437b9a

mec force-pushed the feature/2931-actual-refund-comment-csv-imoprt-row branch from 0556124 to a437b9a Compare December 13, 2023 16:04

WIP: What a mess

eaf63ad

mec closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(2931) Actual, Refund and Comment csv import row #2281

(2931) Actual, Refund and Comment csv import row #2281

mec commented Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023

mec Dec 13, 2023 •

edited

mec Dec 13, 2023

CristinaRO Dec 13, 2023

mec Dec 13, 2023 •

edited

CristinaRO Dec 13, 2023

mec Dec 13, 2023 •

edited

mec commented Dec 20, 2023

(2931) Actual, Refund and Comment csv import row #2281

(2931) Actual, Refund and Comment csv import row #2281

Conversation

mec commented Dec 13, 2023

Changes in this PR

Screenshots of UI changes

Before

After

Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mec Dec 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mec Dec 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mec Dec 13, 2023 • edited

Choose a reason for hiding this comment

mec commented Dec 20, 2023

mec Dec 13, 2023 •

edited

mec Dec 13, 2023 •

edited

mec Dec 13, 2023 •

edited