-
Notifications
You must be signed in to change notification settings - Fork 2
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
(2931/2) Model csv actual refund comment row from csv #2293
(2931/2) Model csv actual refund comment row from csv #2293
Conversation
1934128
to
31b8f3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with this as we went through it in detail together, just one question on a file we didn't go over
activity_actual_refund_comment: | ||
errors: | ||
default: | ||
required: Is required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct for 'required' to be nested in 'default'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it might read a bit strange! The idea is that default
is not any specific value and required
means the value will be required, there are a number of attributes that share this validation, we could repeat the required
for each attribute but that felt like repetetion?
I am hoping someone with better content design will go over all these new error messages at some point - so I'll wait and see if the messages need to be different.
We've already added the `Import::Csv:Financial` class for handling the financial values from csv, actuals and refunds. This commit adds a representation of the row as we expect it to be supplied in the csv, we perform a set of validations adding to the errors instance variable for use later. Like financial values, we treat 'empty' values such as spaces as nothing rather than included values that users would not see. At this point a valid `Import::ActivityActualRefundComment::Row` still may not result in a record being created as validation occurs later and is collected together for presentation to the user. We are doing more validation than the existing improter does, earlier, we feel like this approach results in a number of benefits including: - the code is more approachable, with as many validation happening as early as possible in one place - performant, we don't have to waste resources during the import if we already know the row is invalid - we can offer more helpful errors than the standard model validation might. As part of this work we are introducing some better name and namespacing for the code that relates to the importing processes. For related models, services and any other we can use: `import::csv:activity_refund_comment` We settled on this as the code relates so many models, including: - Actual Transactions - Refund Transactions - Comments on Activities This model reflects the change to the expectation on users around what values should be included in a csv row once it is in use.
31b8f3c
to
5b7a97f
Compare
Now we have a reliable model of a financial value in a csv import, we can move onto the rest of the data in a row.
This is the Actual, Refund and Commnet csv row model, it is responsible for providing a reliable interface to import a row of data that result in a new Actual, Refund or Comment and will form the base of a new importer.
The motivation here is to allow users to provide a comment in the upload when there is no actual or refund to attach that comment to - instead the comment is attached directly to the activity the row represents - this is a common user feature request that would commonly be used to explain the lack of actual or refunds in a report.
We wanted to approach this like the 'form object' design pattern, it is a standin for a model that validates in a specific context, here it is multiple models and the context is importing.
Like a form object, we can offer specific validation and errors to pass back to the user instead of relying on the underlying model validation, the main difference is that here, we do not save the records.
There is a lot of validation during this import, to summerise:
The model can be intialised from a
CSV::Row
instance and can then calledvalid?
much like an ActiveModel. Callingvalid?
will populate the errors for use later in the import process using the same interface as the existing import.It is worth noting that a valid
Import::Csv::ActivityActualRefundComment::Row
may not result in a database record, we have purposly left out any validation that involved loading another object from the database - this will allow for better perfromance as we only need to fetch related object if the row already looks valid.The existing importer relies on the column position whereas this new one relies on the column headers, this is a significant change that is easily missed.
This work is the second in a series, next step: add a service that further validates and creates the database objects if valid.
If you want more context about the longer plan - just ask! :)
https://trello.com/c/lBZkYq5c