-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add register stations #18
Changes from all commits
defa749
ca684c5
71c0980
675503d
09239ee
86ecc7c
e301134
ed74b9d
339a5f1
354dcad
9ea28cc
a8f93a8
6033cc9
1211ec4
703771b
7f7acc7
d1ae475
c1d0b87
728b116
14623af
ba32947
8e7e080
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,3 +2,4 @@ | |
|
||
require_relative 'endpoints/current' | ||
require_relative 'endpoints/one_call' | ||
require_relative 'endpoints/stations' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# frozen_string_literal: true | ||
|
||
module OpenWeather | ||
module Endpoints | ||
module Stations | ||
def register_station(options = {}) | ||
OpenWeather::Models::Station.new(post('stations', options)) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# frozen_string_literal: true | ||
|
||
module OpenWeather | ||
module Models | ||
class Station < Model | ||
property 'id', from: 'ID' # internal identifier for the station | ||
property 'external_id' # external identifier for the station | ||
property 'name' # name of the station | ||
property 'latitude' # geographical coordinates of the location (latitude) | ||
property 'longitude' # geographical coordinates of the location (longitude) | ||
property 'altitude' # height of station above sea level | ||
property 'created_at' # timestamp when station was created | ||
property 'updated_at' # timestamp when station was updated | ||
property 'rank' # rank of station | ||
|
||
def register! | ||
data = OpenWeather::Client.new.register_station(to_h) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For future update, we can save the extra instance construction with |
||
update_attributes!(data) | ||
|
||
self | ||
end | ||
end | ||
end | ||
end |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'spec_helper' | ||
|
||
RSpec.describe OpenWeather::Models::Station do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably shorter to use the same VCR here. The problem with the double is that it will continue working even if we break the API call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, since that client method is unit-tested already, wouldn't it be sufficient to unit-test this as well (check that we call client with the right params)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's sufficient, but implies behavior and IMHO hides bugs. I've debugged my fair share of |
||
include_context 'API client', endpoint: 'https://api.openweathermap.org/data/3.0' | ||
|
||
describe '.register!' do | ||
let(:create_attributes) do | ||
{ | ||
external_id: 'SF_TEST001', | ||
name: 'San Francisco Test Station', | ||
latitude: 37.76, | ||
longitude: -122.43, | ||
altitude: 150 | ||
} | ||
end | ||
|
||
before do | ||
allow(OpenWeather::Client).to receive(:new).and_return(client) | ||
end | ||
|
||
it 'registers a station via the Client', vcr: { cassette_name: 'stations/register_success' } do | ||
model = described_class.new(create_attributes) | ||
result = model.register! | ||
expect(result.object_id).to eq(model.object_id) | ||
expect(result).to have_attributes(create_attributes.merge(id: '5ed21a12cca8ce0001f1aef1')) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'spec_helper' | ||
|
||
RSpec.describe 'stations' do | ||
include_context 'API client', endpoint: 'https://api.openweathermap.org/data/3.0' | ||
|
||
it 'registers a station', vcr: { cassette_name: 'stations/register_success' } do | ||
data = client.register_station( | ||
external_id: 'SF_TEST001', | ||
name: 'San Francisco Test Station', | ||
latitude: 37.76, | ||
longitude: -122.43, | ||
altitude: 150 | ||
) | ||
expect(data).to be_a(OpenWeather::Models::Station) | ||
expect(data).to have_attributes( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does fail, but perhaps with a more cryptic error:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So that's better, it lets us check all attributes. I wouldn't mind if all the other tests changed ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can I open a separate PR for that 🤣 |
||
id: '5ed21a12cca8ce0001f1aef1', | ||
external_id: 'SF_TEST001', | ||
name: 'San Francisco Test Station', | ||
latitude: 37.76, | ||
longitude: -122.43, | ||
altitude: 150 | ||
) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.shared_context 'API client', shared_context: :metadata do | ||
RSpec.shared_context 'API client', shared_context: :metadata do |params| | ||
before do | ||
OpenWeather::Config.reset | ||
OpenWeather::Config.endpoint = params[:endpoint] if params&.key?(:endpoint) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a future PR, I'd like to make it future proof able to write |
||
end | ||
let(:client) { OpenWeather::Client.new(api_key: ENV['OPEN_WEATHER_API_KEY'] || 'api-key') } | ||
end |
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.
Care to explain?
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.
A symbol-keyed hash was “ignored” by the model, I think because a Hashie::Dash property will account for whether it’s defined as a string or symbol (though there didn’t seem to be an issue retrieving the property via a method)
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.
it's possible that the model class needs a merge initializer instead. Either way make sure to have a spec for this and we can fix it later.
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.
Finding this hard to test on Model itself, would it be sufficient to test Station for now?
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.
Yes.