phildini / logtacts

Better contact management.
https://www.contactotter.com
MIT License
61 stars 19 forks source link

Only show contacts that the user is allowed to see, in tag view #2

Closed paulproteus closed 8 years ago

paulproteus commented 8 years ago

Fixes #1

paulproteus commented 8 years ago

Notes for @phildini

All pull requests I submit for merging against the main repo are (C) Asheesh Laroia and distributed under the same terms as phildini/logtacts, which at the moment is the MIT License. If you want a differently-phrased copyright statement from me at any point, I'd be happy to see if I can do that; just ask.

paulproteus commented 8 years ago

Also I wasn't initially sure how to run the tests locally, but thinking about it, maybe it's easy since you do things in Django-standard ways. I can go remember how that works.

paulproteus commented 8 years ago

I believe I can run the tests locally now, so please wait about 20 min before reviewing so I can add a test. I'll close the pull request for now to indicate that it's not ready without a test.

paulproteus commented 8 years ago

I see now that Travis-CI does run the tests!

@phildini I've added a non-working test for this. Don't merge as-is, alas.

Particularly I run the tests by doing:

SLACK_WEBHOOK_URL=http://example.com/ MANDRILL_KEY=party ./bin/python manage.py test

and I get this output at the end.

======================================================================
ERROR: test_contact_list_view_renders_no_philip (contacts.tests.test_views.TaggedContactListViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/logtacts/contacts/tests/test_views.py", line 37, in setUp
    self.response = views.contact_views.TaggedContactListView.as_view()(request)
  File "/tmp/logtacts/local/lib/python2.7/site-packages/django/views/generic/base.py", line 68, in view
    return self.dispatch(request, *args, **kwargs)
  File "/tmp/logtacts/contacts/views/__init__.py", line 14, in dispatch
    return super(LoggedInMixin, self).dispatch(request, *args, **kwargs)
  File "/tmp/logtacts/local/lib/python2.7/site-packages/django/views/generic/base.py", line 88, in dispatch
    return handler(request, *args, **kwargs)
  File "/tmp/logtacts/local/lib/python2.7/site-packages/django/views/generic/list.py", line 174, in get
    context = self.get_context_data()
  File "/tmp/logtacts/contacts/views/contact_views.py", line 183, in get_context_data
    context['tag'] = Tag.objects.get(id=self.kwargs.get('pk'))
  File "/tmp/logtacts/local/lib/python2.7/site-packages/django/db/models/manager.py", line 122, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/tmp/logtacts/local/lib/python2.7/site-packages/django/db/models/query.py", line 387, in get
    self.model._meta.object_name
DoesNotExist: Tag matching query does not exist.

----------------------------------------------------------------------
Ran 21 tests in 0.109s

FAILED (errors=1)

I'm attempting to integrate FactoryBoy and Django and ManyToMany by following these documents:

but I might be doing something wrong, so I figure at this point, I request your feedback and/or you just fixing it for me.

Also I fixed my implementation by writing this test, at least!

phildini commented 8 years ago

FWIW, I think this is looking really good, and I appreciate you making the PR.

My general thoughts are that you might try doing things "the hard way" ie, manually add the tags in the test case, rather than trying to get the auto-creation to work. Let me know if that doesn't make sense, or if you want me to take a deeper look.

Also, your process has elucidated to me how necessary it is to have a good README with how to get testing working. If you want to submit a PR for that, great! Otherwise I'll do that tomorrow most likely.

phildini commented 8 years ago

Check my last commits, see if any of that helps you.

paulproteus commented 8 years ago

"manually" meaning "via interacting with the web app"?​ I can automate that via Django requests in the test, if that's what you mean. Having said that, I think that having a Factory for this would be so glorious, but I can live with doing it in a Factory-less way.

And thanks w/r/t README, although I personally can endure many hardships on the way to submitting patches and not be super fazed.

phildini commented 8 years ago

I meant more: In the test case, add tags to the contact by calling the factories, rather than adding a tag auto-constructor to the Contact factory.

paulproteus commented 8 years ago

Ah hah, that makes sense.​ I can try that next.

phildini commented 8 years ago

I'm going to close this since it's now fixed.

paulproteus commented 8 years ago

Thanks!