search

glossier / solidus_retail

Solidus Extension to Support Retail Operations
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Generate a initializer file when installing the gem #42

Closed thisiscab closed 7 years ago

thisiscab commented 7 years ago

This would resolve https://github.com/glossier/solidus_retail/issues/39

I felt like that there was a need to create a configuration object that would be configured in the host app from an initializer.

thisiscab commented 7 years ago

@braidn @DanielWright @bryanmtl :D Thank you!

thisiscab commented 7 years ago

I am unsure how to deal with default values, so that the specs can at least run on locally and on Travis.

Since Shopify doesn't allow a "test" shop we have to use our own credential.

DanielWright commented 7 years ago

As part of the test setup, can you generate an initializer?

thisiscab commented 7 years ago

Then we would have to explicitly ask the gem user to enter its credential somewhere, there is an extra layer of overhead that we need to have, I am just unsure of the best way to put it.

DanielWright commented 7 years ago

Imagine yourself integrating this extension into an app (and try to forget you've built the extension yourself). What would be the most compassionate way to capture their credentials? I think a good first step would be to generate an initializer that demonstrates how one might configure the extension. Think how Devise creates an initializer with sensible defaults. Does that make sense?

thisiscab commented 7 years ago

Have you looked at the PR, isn't it what we are already doing?

I see your point that for the gem itself, we should generate an initializer which I agree. My counter-argument is that there are no "sensible defaults" the user of this extension will still have to enter their Shopify API key and Token in order to run the specs.

DanielWright commented 7 years ago

Ah! I misunderstood your question, which is a shame cos it's a really really good one. Hmm.

I don't really like the way solidus_gateway provides its own credentials, because I've actually run into a bug because of it (the API version those credentials run is out-of-date, which confounded me for days).

I think we have to pull them from ENV for the specs. We can't guarantee any credentials we hard-code will remain functional. So, what do you think about depending on ENV vars, but with a human-friendly error-message if they're not found?

Something like:

begin
  ENV.fetch('SHOPIFY_API_KEY')
rescue KeyError
  puts "You must set the following environment variables: ... Please see <a href>the documentation</a> for ways to accomplish this."
end
thisiscab commented 7 years ago

What do you think of the changes? @DanielWright

braidn commented 7 years ago

Also @cabouffard you never pinged me here for a CR. Did you read my post from yesterday morning?

braidn commented 7 years ago

Also, it would be super nice to move #39 due to it's conversation into here when creating the PR, unless you don't think that transition makes sense. Helps keep context in one thread IMHO.