Maintenance #37
Maintenance #37
Conversation
- it prevents us from starting the app: cannot load such file -- xmlrpc/client (LoadError) - moved functionality into our own little class with the same API
There was a problem hiding this comment.
Hey @beanieboi – thanks a ton!
I've added some comments; mostly about the givie token change and the Gravatar replacement.
spec/spec_helper.rb
Outdated
| @@ -56,5 +56,5 @@ def stub_stripe_methods | |||
| allow_any_instance_of(Donation).to receive(:stripe_create_charge) | |||
| end | |||
|
|
|||
| ENV['GIVIE_SECRET']="this is the testing secret. it is not very secret" | |||
| ENV['GIVIE_SECRET'] = "2d2dd11b2d4d9c55a38db15ff40ee826" | |||
There was a problem hiding this comment.
Is this a real thing? I would prefer to leave the original value so that we can be sure not to have checked in real tokens
There was a problem hiding this comment.
the requirements for ActiveSupport::MessageEncryptor.new changed, it requires a 32 byte key now. Using the old secret gives a ArgumentError: key must be 32 bytes
I can cut down the original string to 32 chars.
spec/helpers/givie_helper_spec.rb
Outdated
| before do | ||
| ENV['GIVIE_SECRET'] = "kitten sandwich is a strange project name" | ||
| ENV['GIVIE_SECRET'] = "77a115a459b0499bd8719d5143113fe1" |
There was a problem hiding this comment.
Is this a real thing? I would prefer to leave the original value so that we can be sure not to have checked in real tokens
app/models/gravatar.rb
Outdated
| @@ -0,0 +1,36 @@ | |||
| class Gravatar | |||
There was a problem hiding this comment.
Shall we move this to lib/? There are of course different schools about where to put what. Personally, I like lib to be a place that has no ties to the domain of the app and could be used somewhere else without changes. A gravatar generator qualifies for this.
app/models/gravatar.rb
Outdated
| Digest::MD5.hexdigest(email.downcase.strip) | ||
| end | ||
|
|
||
| def extension_for_image(options) |
There was a problem hiding this comment.
We're only using two keys. Can we use kwargs instead of the options hash?
There was a problem hiding this comment.
My goal was to keep the original API. I'm ok with not doing that, I just wanted to keep the changes as minimal as possible. I can rewrite the Gravatar class to only support our usage
app/models/gravatar.rb
Outdated
|
|
||
| def query_for_image(options) | ||
| query = '' | ||
| [:rating, :size, :default, :forcedefault, :r, :s, :d, :f].each do |key| |
app/models/gravatar.rb
Outdated
| @email = email | ||
| end | ||
|
|
||
| def image_url(options = {}) |
There was a problem hiding this comment.
Let's remove the ssl option and use https by default
app/models/gravatar.rb
Outdated
| class Gravatar | ||
| attr_reader :email | ||
|
|
||
| def initialize(email, options = {}) |
There was a problem hiding this comment.
We don't seem to use options. Let's remove it
app/models/gravatar.rb
Outdated
| attr_reader :email | ||
|
|
||
| def initialize(email, options = {}) | ||
| raise ArgumentError, "Expected :email" unless email |
There was a problem hiding this comment.
We can remove the condition by making email a required kwarg. We can rely on the parser to ensure argument correctness:
def initialize(email:)
@email = email
end
|
I'd prefer to leave the review up to someone who understands the campaign app better than me <3 but please ping me if you need anything from my side @beanieboi! |
This is a PR to addresses some minimal maintenance work.
Which includes:
Things that needs to be addressed