[Round Trip Data] A start on importing user data via CSV

[Benratelade]

Created a test for data upload.
Some changes to the html for the Stats page, to allow for better targeting in tests.
Added an ugly upload button to Home.js
Created a support class for handling CSV uploads (and downloads, later).
Also created a sample csv file, which really ought to be replaced with some Factory.

@saramic this is definitely not ready for a proper pull request, but help is needed!

Several things:

• I created a test feature for this, but there are a few things I couldn't figure out. In particular, how do we get those ActionCable notifications to show up. Not sure if there's only Devise doing some magic for now, or if we already have a way of passing our own info to those notifications.
• Ideally we should have a Factory for creating a CSV file (rather than having a static file in our tests)
• I haven't created a route or a controller for the file upload. I'm not sure how you would prefer to build those.
• CSV Files would need to be uploaded and saved somewhere. I can see we serve images from an S3 bucket, so I assume it's the way to go?
• I assume we should have a queue for processing those uploads (otherwise the server would just be hanging during processing)?

[Benratelade]

@saramic I finally have something that works for importing Daily Stats via CSV. Let me know if there is anything we could improve.

[saramic]

we are in react here - I would have just added a plain old button in Rails, it would be like in a separate section - fetch your data - or upload your data on an account setup page? that said you "could" make it more "interactive" by using JS and react

[saramic]

cool - would have been nice in a separate pull request - so that I could merge that in :)

[saramic]

you could do something like

import_file = generate_file_with_contents("health_sample.csv") do
  <<~HEALTH_SAMPLE_CSV
    Date,Activity Name,Quantity
    15/08/2021,Weight,67.1
    14/08/2021,Weight,66.8
    13/08/2021,Weight,66.5
    12/08/2021,Weight,66
    11/08/2021,Pull ups,50
    11/08/2021,Weight,66.5
    10/08/2021,Weight,67
    08/08/2021,Push ups,200
    08/08/2021,Weight,66
    08/08/2021,Push ups,100
  HEALTH_SAMPLE_CSV
end

assuming you have a file_helper.rb somewhere in your spec with code similar to

module FileHelper
  def generate_file_with_contents(filename)
    file = Tempfile.new(filename)
    content = yield
    content = csv_lines_to_string(content) if content.is_a?(Array)
    file.write(content)
    file.close
    file
  end

  private

  def csv_lines_to_string(csv_lines)
    ordered_columns = csv_lines.map(&:keys).flatten.uniq.sort
    CSV.generate do |csv|
      csv << ordered_columns
      csv_lines
        .each do |csv_line|
        csv << ordered_columns.map { |col| csv_line[col] }
      end
    end
  end
end

[saramic]

cool, although we will probably want to move it into a job so that it does NOT execute in a web request but in its own job

[saramic]

probably better to use the app_stats_path url thing although I can never remember which way it is - or should this go back to action: show ?

[saramic]

nice, feels like we could describe more of the process in this "chain of functions"

read_file(filepath)
  .then { |file| read_csv(file) }
  .map { |csv_line| process_csv_line(csv_line) }
  .map { |data| process_data(data) }
  .map { |data| user.daily_stats.create(data) }

maybe? or something similar?

ALSO should the last line be a daily_stats.create ? or should we allow for find_or_create ? - ie to allow updates?

[saramic]

so the "more rails" way to do this would be something like

resource :settings, only: [:show] do
  collection
    post :import
  end
end

another 'nice' alternative would be to ask for the user id /settings/long-user-id/import which later we could allow an "admin" to import / export on behalf of a user - very handy!!!

[saramic]

good start - although we must remember to put this into a page fragment

also I am starting to be more and more anti the "count" test .to be(7) mostly because if it break the first thing I want to see is what you actually got.

in this case our 10 data points across 7 days are completely arbitrary - a good "real data representation" but not specific enough to help with testing. I would say that in BDD (behaviour driven development) a form of "example mapping" (you can look it up in google) is useful to take a real data set and break it down into a minimum set that exercise constraints.

int this case we could have 3 data points, 1 occurs on 1 day and 2 on the other day - which would make this easy to test that they are all there and exactly what is inside

[saramic]

nice I really like this - only comment is that we could bring the "input files" closer to the tests by wrapping each example in a context this would also have the positive side effect that the it statement would no longer need the then clause and there is then no need to name the file as it would be the name of the context. This name of the context also gets displayed when you run the tests OR when they break in CI so if one of the tests failed it would tell you that eg

context "with a file that has the wrong file extension" do
  let(:file) { ... }

  it "raises an unknown file type RUntimeError" do
    ...
  end
end

context "...
  let(:file) {...
...

the fact that each let is file also slightly reduces the load on reading the test as everywhere you see described_class.import(file.path, user.id)

which theoretically, given that described_class is already hiding the name of the class, you could actually change into a subject at the top of the file

say

subject(:import_file) { described_class.import(file.path, user.id) }
...
it "..." do
  import_file
  expect(...)

[saramic]

this is cool but does kind of hide the implementation of what exactly is *filename vs using (filename, file_extension) maybe?