Skip to content
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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Metrics/BlockLength:
Description: allow features to be more descriptive and longer
Exclude:
- "spec/features/**/*"
- "spec/services/**/*"

RSpec/Capybara/FeatureMethods:
Description: allow given/background/scenario in capybara features
Expand Down
1 change: 1 addition & 0 deletions app/assets/stylesheets/components/_flash_message.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
position: fixed;
bottom: 4rem;
right: 4rem;
z-index: 1000;
}
9 changes: 9 additions & 0 deletions app/controllers/settings_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class SettingsController < ApplicationController
def show
end

def import
DailyStatsDataImporter.import(params[:file].path, current_user.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

redirect_to "/app/stats", notice: "Upload complete"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

end
end
5 changes: 5 additions & 0 deletions app/javascript/react/components/Nav/Nav.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ const Nav = () => (
<li className="nav-item">
<span className="nav-link disabled">Ranking</span>
</li>
<li className="nav-item">
<a href="/settings" className="nav-link">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm so jumping from React to Rails - should we "duplicate" the nav inside of rails to have it consistent?

Settings
</a>
</li>
</ul>
);

Expand Down
56 changes: 30 additions & 26 deletions app/javascript/react/pages/Home/Home.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,32 +44,36 @@ const Home = () => {
return (
<div>
<h1 className="my-4">Are you sweating enough?</h1>
<button
type="button"
className={`btn btn-${display ? "danger" : "success"}`}
onClick={() => setDisplay((prev) => !prev)}
>
{display ? "hide stats" : "add stats for today"}
</button>
{display && (
<>
<div className="my-4">
<SingleDatePicker
date={date}
onDateChange={(newDate) => setDate(newDate)}
focused={focus}
onFocusChange={({ focused }) => setFocus(focused)}
id="date"
/>
</div>
<AddStatsForm
date={date.toISOString()}
className="my-4"
onSubmit={submitHandler}
formInput={formInput}
/>
</>
)}
<div className="row">
<div className="col-12">
<button
type="button"
className={`btn btn-${display ? "danger" : "success"}`}
onClick={() => setDisplay((prev) => !prev)}
>
{display ? "hide stats" : "add stats for today"}
</button>
{display && (
<>
<div className="my-4">
<SingleDatePicker
date={date}
onDateChange={(newDate) => setDate(newDate)}
focused={focus}
onFocusChange={({ focused }) => setFocus(focused)}
id="date"
/>
</div>
<AddStatsForm
date={date.toISOString()}
className="my-4"
onSubmit={submitHandler}
formInput={formInput}
/>
</>
)}
</div>
</div>
</div>
);
};
Expand Down
4 changes: 2 additions & 2 deletions app/javascript/react/pages/Stats/Stats.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const Stats = () => {
if (error) return <p>{`Error! ${error.message}`}</p>;

return (
<ul className="list-group">
<ul className="list-group daily-stats-list">
<li className="list-group-item">
<div className="row">
<div className="col-md-2">
Expand All @@ -31,7 +31,7 @@ const Stats = () => {
</div>
</li>
{data.dailyStats.map((stat) => (
<li key={stat.id} className="list-group-item">
<li key={stat.id} className="list-group-item daily-stats-item">
<div className="row">
<div className="col-md-2">{moment(stat.date).format("L")}</div>
<div className="col-md-10">
Expand Down
63 changes: 63 additions & 0 deletions app/services/daily_stats_data_importer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
require "csv"

class DailyStatsDataImporter
def self.import(filepath, user_id)
new.import(filepath, user_id)
end

def import(filepath, user_id)
user = User.find(user_id)
read_file(filepath)
.map { |data| process_data(data) }
.map { |data| user.daily_stats.create(data) }
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


private

def read_file(filepath)
case File.extname(filepath)
when ".csv"
read_csv(filepath)
else
raise "Unknown file type"
end
end

def read_csv(filepath)
results = []
CSV.foreach(filepath, headers: true) do |row|
row_data = {
date: row["Date"],
data: {
row["Activity Name"] => row["Quantity"],
},
}
same_date_item = results.find do |item|
item[:date] == row_data[:date]
end
if same_date_item.present?
same_date_item[:data] = same_date_item[:data].merge(row_data[:data])
else
results << row_data
end
end
results
end

def process_data(data)
parse_date(data)
.then { |new_data| parse_values(new_data) }
end

def parse_date(data)
data[:date] = Date.parse(data[:date])
data
end

def parse_values(data)
data[:data].each do |key, value|
data[:data][key] = value.to_f
end
data
end
end
8 changes: 8 additions & 0 deletions app/views/settings/show.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div class="row">
<div class="col-12">
<%= form_with(url: import_path, multipart: true) do |form| %>
<%= form.file_field :file, {accept: ".csv"} %>
<%= form.button "Upload Stats", {class: "btn btn-info"} %>
<% end %>
</div>
</div>
2 changes: 1 addition & 1 deletion config/initializers/devise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@
# ==> Configuration for :timeoutable
# The time you want to timeout the user session without activity. After this
# time the user will be asked for credentials again. Default is 30 minutes.
# config.timeout_in = 30.minutes
config.timeout_in = 2.weeks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# ==> Configuration for :lockable
# Defines which strategy will be used to lock an account.
Expand Down
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
root to: "users#index"
end
mount GraphiQL::Rails::Engine, at: "/graphiql", graphql_path: "/graphql" if Rails.env.development?
resource :settings, only: [:show]
post :import, to: "settings#import"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!!!

post "/graphql", to: "graphql#execute"
devise_for :users
# For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html
Expand Down
26 changes: 5 additions & 21 deletions spec/features/user_adds_stats_using_nice_ui_spec.rb
Original file line number Diff line number Diff line change
@@ -1,29 +1,13 @@
require "rails_helper"

feature "User adds stats using nice UI", js: true do
context "when a user existst with stats" do
context "with a user who has stats" do
before do
@user_claudia = create(:user_claudia)
@user_claudia
.daily_stats
.append(
create(
:daily_stat,
date: Date.parse("2015-04-01"),
data: {situps: 100, weight: 66.6},
user: @user_claudia,
),
)
@user_claudia
.daily_stats
.append(
create(
:daily_stat,
date: Date.parse("2015-04-02"),
data: {situps: 80, weight: 66.6},
user: @user_claudia,
),
)
@user_claudia.daily_stats.append(create(:daily_stat, date: Date.parse("2015-04-01"),
data: {situps: 100, weight: 66.6}, user: @user_claudia,))
@user_claudia.daily_stats.append(create(:daily_stat, date: Date.parse("2015-04-02"),
data: {situps: 80, weight: 66.6}, user: @user_claudia,))
end

scenario "Claudia signs in, sees her stats and updates them" do
Expand Down
71 changes: 71 additions & 0 deletions spec/features/user_uploads_own_data_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
require "rails_helper"

feature "User uploads their data using rails UI", js: true do
context "with a user that has no stats" do
before do
@user_claudia = User.create!(
email: "[email protected]",
password: "1password",
confirmed_at: DateTime.now,
)
end

scenario "Claudia signs in, sees no stats in Stats" do
When "Claudia signs in and views her stats in pretty UI" do
visit root_path
page.find("nav a", text: "Log in").click
focus_on(:form).for(user_session_path).submit(
"Email" => @user_claudia.email,
"Password" => @user_claudia.password,
)

page.find(".nav a[href='/app/stats']", text: "Stats").click
end

Then "she sees no stats" do
expect(
page
.find_all(".daily-stats-list .daily-stats-item")
.size,
).to be(0)
end

When "she uses the Rails UI to upload a csv of stats" do
page.find("[data-widget-type='stats-app'] .nav-tabs a[href='/settings']", text: "Settings").click
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
attach_file("file", import_file.path)
page.find("button", text: "Upload Stats").click
end

Then "she sees a notification that the upload is completed" do
wait_for do
page.find("p.alert [data-testid=\"message\"]").text
end.to eq "Upload complete"
end

Then "she sees the new stats under stats" do
page.find(".nav a[href='/app/stats']", text: "Stats").click

expect(
page
.find_all(".daily-stats-list .daily-stats-item")
.size,
).to be(7)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
end
end
3 changes: 3 additions & 0 deletions spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,7 @@
# predictable host and port for email links
Capybara.server_port = 3001
Capybara.server_host = "localhost"

# file helper can create temporary files with given content
config.include FileHelper
end
Loading