Skip to content
This repository has been archived by the owner on Apr 17, 2020. It is now read-only.

Spree 1.0 + Fully Tested #12

Closed
wants to merge 70 commits into from

Conversation

iloveitaly
Copy link
Member

This is combination of the work that has been done by @danrasband, @gfmurphy, and others.

All test are passing (bundle exec rspec).

Removed a lot of unneeded overrides.

divineforest and others added 30 commits January 31, 2012 13:35
* Versionfile: added 1.0
* checkout.js removed since it copies spree origin checkout.js
* models, controllers, views namespacing
* Refactored: don't replace original checkout form. Just prepend our choose address subform
* CheckoutHeper removed (not needed after refactoring)
* Order decorations removed (seems not needed for spree 1.0)
…ound. safer. helps not break spree order factories.
iloveitaly and others added 6 commits May 28, 2012 08:25
* address_field's id_prefix is shortened to one character.
  This makes id naming consistent with spree's checkout/_addresses form
* Alternative Phone preference now pulled from Spree::Config
* Country field formatting the same as Spree default
* Removed blank p tag above address fields
* Removed unneeded preferences
@iloveitaly
Copy link
Member Author

Sorry about the number of commits: I should of broken this down into multiple pull requests (and still can if needed). There are still some minor changes I'd like to make to this extension, but it has good test coverage and is working fine on spree 1.1 at this point.

I'd love to hear your feedback on what needs to be done to get these changes merged into master (broken into separate pull requests, ripping out specific changes, etc). The goal with these changes was to pull together the community's work on this extension and merge it back into master so the codebase of this extension does not end up fragmented.

Here is a more comprehensive list of what is changed in this pull request:

Misc

  • Using change in migration instead of up & down. Migration assumes that table is named spree_addresses (because of namespacing extension is only spree 1.0 compatible anyway)
  • Gemfile.lock should not be a Gem's repo per yedukat's article
  • Ability to add addresses to account in the "My Account" area
  • Moved helper methods into an AddressBook helper to fix some method not found errors (thanks @frahugo)
  • Rails 3.2 / Spree 1.1 mass-assignment fixes
  • Address form element IDs match the spree's address form. This is important because any checkout.js that spree uses will continue to work (e.g. the state + country drop-down toggle)
  • Fixed issue in Spree::Address#destroy that would cause an error to be thrown (using super instead of destroy_without_saving_used)
  • <br/> instead of newlines in Spree::Address#to_s. Makes for cleaner looking address listing on checkout#address
  • Removed unneeded AddressBookConfiguration preferences: using Spree::Config alternative phone preferences in addresses/_form
  • Removed alternative_phone from ADDRESS_FIELDS, this is handled manually because of the integration with Spree::Config
  • Fixed bad partial reference in addresses/edit
  • Using language definitions instead of static text / auto-generated text in a couple places

Testing

  • Removed cucumber tests, using rspec instead (rspec is used exclusively in the spree core project – makes sense to keep things specific)
  • Better test converage. Model + cabybara controller tests will ensure that this extension continues to work as spree evolves. The "My Account" functionality still needs a bit more converage though.
  • Using new FactoryGirl syntax (Factory.create vs FactoryGirl.create)
  • rspec + growl + guard for easy testing
  • Proper JS include (store/spree_core) for passing cabybara JS validation tests

@iloveitaly
Copy link
Member Author

@romul @jumph4x any thoughts on the changes here? My main motivation to get this merged into master is to bring the various forks of this project into one heavily tested (very important to myself since this extension modifies core spree functionality) master branch that people can work off of.

If you need me to break this up into multiple pull requests, let me know.

@jumph4x
Copy link
Contributor

jumph4x commented Jun 25, 2012

@iloveitaly Assigned @romul to take a look at this within a week or so.

@@ -1,14 +1,17 @@
Redistribution and use in source and binary forms, with or without modification,
Copyright (c) 2012 [name of plugin creator]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all modifications to this file.

@iloveitaly
Copy link
Member Author

Thanks for the code review @radar. I'll make the changes soon.

@romul
Copy link
Collaborator

romul commented Jul 3, 2012

@iloveitaly, can you rebase your changes on current master branch? Currently this branch looks too disorderly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants