alexrothenberg / motion-addressbook

MIT License
89 stars 30 forks source link

Adds `AB.create` to show the `ABNewPersonViewController` #58

Closed colinta closed 10 years ago

colinta commented 10 years ago

Code is very similar to AB.pick. I'd like to get specs in here, so no need to merge unless you're feeling lucky, but wanted to get this on your radar in the meantime.

colinta commented 10 years ago

rake spec is now inexplicably failing; none of the changes i made were to the core, though... looking into it.

alexrothenberg commented 10 years ago

colin I don't think the failure has anything to do with you. rake spec has been failing for a while and I just haven't had time to look into it (bad me).

I can look into it next week and get it going again.

jmay commented 10 years ago

Checking now if I have any outstanding changes that might fix things. rake spec works fine on my master.

alexrothenberg commented 10 years ago

Thanks jmay.

Travis has been failing for a while https://travis-ci.org/alexrothenberg/motion-addressbook/builds. It'd be nice to push a new version soon too, its been a while.

colinta commented 10 years ago

Yeah if I remove my new specs they pass, but if I include them I get this error:

AB: Could not compile statement for query (ABCCopyArrayOfAllInstancesOfClassInSourceMatchingProperties):
SELECT ROWID, Name, ExternalIdentifier, Type, ConstraintsPath, ExternalModificationTag, ExternalSyncTag, AccountID, Enabled, SyncData, MeIdentifier, Capabilities FROM ABStore WHERE Enabled = ?;

Stackoverflow says its permissions related.

colinta commented 10 years ago

Also guys, what do you think about this:

commit aa0de3213634eb66a1c065b3aa2c05c5873ef021
Author: Colin T.A. Gray <colinta@gmail.com>
Date:   Wed Mar 12 11:09:13 2014 -0600

    keep existing backup, in case the specs fail

diff --git a/spec/ios/helpers/spec_helper.rb b/spec/ios/helpers/spec_helper.rb
index 7d970d1..7b965a2 100644
--- a/spec/ios/helpers/spec_helper.rb
+++ b/spec/ios/helpers/spec_helper.rb
@@ -23,10 +23,14 @@ def protect_existing_address_book

   warn "PROTECTING EXISTING ADDRESS BOOK IN SIMULATOR"

-  `rm -rf \"#{AB_PATH_BAK}\"`
-  `mv \"#{AB_PATH}\" \"#{AB_PATH_BAK}\"`
-  # Kernel.system "rm -rf \"#{AB_PATH_BAK}\""
-  # Kernel.system "mv \"#{AB_PATH}\" \"#{AB_PATH_BAK}\""
+  if File.exists?(AB_PATH_BAK)
+    warn "KEEPING EXISTING BACKUP"
+  else
+    `rm -rf \"#{AB_PATH_BAK}\"`
+    `mv \"#{AB_PATH}\" \"#{AB_PATH_BAK}\"`
+    # Kernel.system "rm -rf \"#{AB_PATH_BAK}\""
+    # Kernel.system "mv \"#{AB_PATH}\" \"#{AB_PATH_BAK}\""
+  end
 end

 at_exit do

I just lost my sim address book because the specs crashed :-( This will prevent that, and restore the address book once it's available again.

jmay commented 10 years ago

The Travis failures are because they haven't updated their RM version in many months. Even when they do, we'll have a problem because this gem requires user to approve access to the AddressBook (new enforced requirement in the simulator as of Mavericks) and I haven't yet found a way to automated granting that approval.

colinta commented 10 years ago

Hmm, ok, should I put these specs somewhere else then and we can run them separately? They run and pass fine when they're run on their own, and just running rake spec passes as well.

$ rake spec others=1
Resolving dependencies...
     Build ./build/iPhoneSimulator-7.0-Development
   Compile ./spec/others/address_book/creator_spec.rb
   Compile ./spec/others/address_book/picker_spec.rb
      Link ./build/iPhoneSimulator-7.0-Development/AddressBook_spec.app/AddressBook
    Create ./build/iPhoneSimulator-7.0-Development/AddressBook_spec.app/PkgInfo
    Create ./build/iPhoneSimulator-7.0-Development/AddressBook_spec.app/Info.plist
    Create ./build/iPhoneSimulator-7.0-Development/AddressBook_spec.dSYM
  Simulate ./build/iPhoneSimulator-7.0-Development/AddressBook_spec.app
PROTECTING EXISTING ADDRESS BOOK IN SIMULATOR
KEEPING EXISTING BACKUP
AddressBook::Picker

IOS UI for creating an entry
  - should yield the created person
  - should yield nil if canceled

AddressBook::Picker

IOS UI for finding people
  - should yield the selected person
  - should yield the selected person
  - should yield nil when cancelled

5 specifications (13 requirements), 0 failures, 0 errors
RESTORING ORIGINAL ADDRESS BOOK IN SIMULATOR
colinta commented 10 years ago
$ rake spec
# ...
  Simulate ./build/iPhoneSimulator-7.0-Development/AddressBook_spec.app
PROTECTING EXISTING ADDRESS BOOK IN SIMULATOR
AddressBook::Group
# ...

85 specifications (191 requirements), 0 failures, 0 errors
RESTORING ORIGINAL ADDRESS BOOK IN SIMULATOR
jmay commented 10 years ago

Good idea on the backup, I should have thought of that before.

jmay commented 10 years ago

@colinta - try moving AB.create into the AddressBook class itself (lib/motion/address_book.rb). Then it won't be possible to invoke without ensuring that AB access permission has been granted beforehand.

colinta commented 10 years ago

Done, moved the AB.pick method there, too.

Ran rake spec && rake spec osx=1 && rake spec others=1 with no failures.

colinta commented 10 years ago

I tried something, made some progress but it didn't resolve things. If I run the controller specs before person_spec.rb, they pass and all the tests run, but I do still get the same error as before (Could not compile statement for query) and some of the specs in person_spec.rb fail.

The sorting specs end up with @ab.people being an empty array, and the notifications spec (which is where the error message happens) says 2 != 4, I didn't look into what that means.

jmay commented 10 years ago

Did that help? Meaning, do the specs run when all consolidated? I've been tussling with these permissions issued for ages, any improvement on the current hack would be welcome.

jmay commented 10 years ago

Re the 2 != 4 stuff, some of the specs might be assuming that the simulator AB starts out empty, so if another spec doesn't clean up properly then you'll get errors in spurious places. These tests ought to be more resilient.

colinta commented 10 years ago

Nope, the specs still don't pass after being consolidated.

jmay commented 10 years ago

This is really bothering me.

These all work:

rake spec files=creator_spec
rake spec files=creator_spec,person_spec
rake spec files=person_spec
rake spec files=creator_spec,group_spec
rake spec files=group_spec

But this causes that "could not compile" output:

rake spec files=creator_spec,person_spec,group_spec
jmay commented 10 years ago

Making progress, of a sort. Narrowed down that "could not compile" message to the specific line in the test suite. Still baffled as to why it is occurring - it isn't a permissions issue.

colinta commented 10 years ago

nice! that's progress, though. what line?

jmay commented 10 years ago

In the "notifications" section, always the notification driven for the first record creation.

I think there are some deep issues with how iOS deals with having multiple AB instances. I just ripped out every AddressBook::Person.new in the test suite and replaced with @ab.new_person and likewise for create. That may have just cleared up all the problems. I think it's time to deprecate everything that doesn't go through an AddressBook::AddrBook instance: I don't understand what is going on inside iOS but otherwise I think we'll continue to see these bizarre messages.

Patch coming shortly for you to try with Creator.

jmay commented 10 years ago

just pushed to my fork https://github.com/jmay/motion-addressbook

only change is person_spec.rb

jmay commented 10 years ago

hang on, there's other stuff on my master, let me untangle

jmay commented 10 years ago

I've apparently been sitting on some extra commits for a while. All internal changes & refactoring, and it all looks clean, but it does cause merge conflicts with your Creator patch.

jmay commented 10 years ago

@colinta I pushed all the changes through to master, and included your creator.rb. Still trying to get your picker spec to work, will be great to get that old issue cleared out.

colinta commented 10 years ago

Looks like it's all merged in! sweet! :-D