feat: expose ProfitSharePasses#32
Conversation
joshmcclain
left a comment
There was a problem hiding this comment.
Great work @josephineweidner ! Some minor idiomatic comments but LGTM overall!
|
FYI I'm probs gonna change this to use JSONAPI::Serializer now that I know a little more about ember so that's why I haven't merged it in (in addition to the dependency stuff which I'll resolve after #33 is merged) @joshmcclain |
@josephineweidner -- a widely misunderstood thing about ember is that you need to use their serializer / deserializer / ember-data store to get API data into the frontend. You can simply make a fetch request from the route.js file instead: I'd highly recommend this approach over adding the clunky / heavy duty machinery that is JSONAPI::Serializer to rails, etc etc |
|
|
||
| def internals_budget_multiplier | ||
| (snapshot && snapshot["inputs"]["internals_budget_multiplier"]) || nil | ||
| end |
There was a problem hiding this comment.
A few notes here:
- There's inconsistency in the naming convention of these methods, some end with
_from_snapshotand some don't - we shouldn't fallback to
nil, rather a type that corresponds to the type returned in a good case (so the FE doesn't need to do nil checks (this is more of a sanctuary best practice than ruby dogma) - a good way to dig through hashes with string keys in Ruby is using
dig, for example:snapshot.dig("inputs", "fica_tax_rate") || 0 - a good way to wrap a chained method in a nil check is to use
try, for example:snapshot.try(:dig, ["inputs", "fica_tax_rate"]) || 0 - that said, Ruby's safe access operator probably beats
trybecause wrapping a chained method feels unergonomic, for example:snapshot&.dig("inputs", "fica_tax_rate") || 0 - finally, logic used soley for rendering a model should be entirely in the serializer file (think of the serializer as a view, just like a HTML/XML file), not the model file (your serializer methods can do the above digging directly, rather than calling a model method)!
There was a problem hiding this comment.
Sorry @hhff - this code wasn't ready for re-review, I was in "just make it work" mode after hooking it up to the ember side. Your notes are v helpful nonetheless and I'll be sure to address them!
Marking this PR as draft until it's actually ready for your eyes again. Thanks!
There was a problem hiding this comment.
Re fallback vals: to_f takes care of this for us (which we need to do anyway bc some snapshots start storing values as strings instead of floats). Not as explicit as || 0 but it works and I think .to_f and a fallback would be misleading about to_f behavior. I can add a comment if this isn't obvious to Ruby devs?
irb(main):001:0> nil.to_f
=> 0.0
| @@ -0,0 +1,6 @@ | |||
| Rails.application.config.middleware.insert_before 0, Rack::Cors do | |||
| allow do | |||
| origins 'http://localhost:4200' | |||
There was a problem hiding this comment.
Let's make this origins '*', I'd like this data to be publicly accessible to all websites
There was a problem hiding this comment.
Done, but limited it so that only profit_share_passes can be accessed. Sound good?
joshmcclain
left a comment
There was a problem hiding this comment.
A few minor comments but LGTM overall!
This is part of this task. The TLDR is that we want to update https://profit.sanctuary.computer/ to pull from real data in stacks. This PR scaffolds an endpoint to surface that data.
You can pull down this branch and hit this at
http://localhost:3000/api/profit_share_passes:Response