Stuff we need to look at

[manmartinez]

• [x] Why do we need solidus_social? I think it allows users to sign in through amazon, but I'm not even sure if it works
• [x] Refunding a transaction from the spree admin doesn't work, the button is just sitting there pointing to '#'
• [ ] We should probably remove all Amazon copyright notices.
• [x] We need to update the gemspec file to set the author and description
• [x] Get to at least 90% on test coverage
• [x] Storing configuration inside Spree::AppConfiguration won't work if use_static_preferences! is being used, we should probably have our own configuration class
• [x] Revisit the Spree::AmazonTransaction idea, right now an order has_many amazon transactions, and a payment also has an amazon transaction as a source, so in the best case scenario we end up with two amazon transactions per order, which doesn't make much sense to me...
• [x] Refactor Spree::AmazonController. Although I've done some work there, there's still a lot of logic in that controller we probably want to extract some classes to take care of updating the spree order through checkout
• [ ] Refactor Spree::Gateway::Amazon looks pretty terrible :panda_face: ← That's a sad panda
• [x] Take a look at closing payments. Apparently we've added the close event to a payment, but we also added a new method on the processing module called close! those two are overriding each other, probably not what we want

[mtomov]

It's great that you've outlined the problems, as this is very time consuming.

I think that we'd need to look at

6) Storing configuration inside Spree::AppConfiguration won't work if use_static_preferences! is being used, we should probably have our own configuration class

sooner rather than later, as I'd immediately switch to use static preferences, and lot's of time can be wasted trying to debug this. I guess we need have a look how other solidus extensions do it.

[kenton]

All good stuff. A few thoughts.

1. Yeah, I think we need solidus_social for the exact reasons you mentioned. When I tried without it, it didn't work. Can't remember what the failure was, but I think it wouldn't let me login.
2. If we can't refund, should we even use this. Can we at least refund via the console...is this just a matter of wiring up the admin view or do refunds simply not work at all?
3. through 5. agree on all points
4. Agree. I think this might be what was causing me issues locally, with those Amazon settings being deleted from the DB, seemingly at random. Wonder why they didn't put this stuff in a .env file, secrets file or something similar?
5. through 10. agree on all points

So @manmartinez, can you add these as individual tickets on the TSP board in jira? Some of these will be higher priority than others, but we should keep track of all of this. It's all good dev work that can / should be done at some point.

Thanks for the help on this.

[manmartinez]

@mtomov Yeah configuration is a big deal, I handled this on another spree extension by inheriting from Spree::Preferences::Configuration you can take a look here if you think this works I'll make a PR

@kenton:

2) If we can't refund, should we even use this. Can we at least refund via the console...is this just a matter of wiring up the admin view or do refunds simply not work at all?

I think refunds can still be made by logging into amazon payments and refunding from there.

4) Agree. I think this might be what was causing me issues locally, with those Amazon settings being deleted from the DB, seemingly at random. Wonder why they didn't put this stuff in a .env file, secrets file or something similar?

They provide an admin UI to setup amazon, if we want to move to secrets.yml configuration should be moved to an initializer and the admin UI should be removed, I'm fine with that @mtomov any thoughts?

[hectoregm]

@manmartinez I am interested in the opinions around the configuration because braintree appears to be in the same situation where the config is set in the admin UI, my idea is by default tries to use ENV variables if there are not set fallback to admin UI configuration.

[manmartinez]

@hectoregm I think having both might be confusing I'd rather have either the admin UI or an initializer

[hectoregm]

@manmartinez yeah supporting both is confusing

[mtomov]

@mtomov Yeah configuration is a big deal, I handled this on another spree extension by inheriting from Spree::Preferences::Configuration you can take a look here if you think this works I'll make a PR

Yes, I think this works. Found a similar example for Solidus Globalize, so this should be the correct way to do it.

I don't really understand why just decorating the App configuration doesn't work, but I trust you. It might be because of the load order and them being in-memory cached.

I think that if the Admin UI is already there, and if we just need to set-up those preferences correctly, then that would be preferable over storing them in an initializer.

[manmartinez]

@hectoregm, @mtomov ok so we'll keep the admin UI and store the configuration on the database by inheriting from Spree::Preferences::Configuration I'll make a PR