Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Add getting contact by field #29

Merged
merged 17 commits into from Feb 9, 2015

Conversation

rudigiesler
Copy link

We want to be able to get a contact by specifying a specific field, for example msisdn. The queries should have the form ?q='msisdn=+1234'

@rudigiesler rudigiesler self-assigned this Feb 6, 2015
@SteveBarnett
Copy link

@rudigiesler Do we match for a missing / present +?
I mean if someone searches for +123, would they see 123. And if they searched for +123 would they see 123 (i guess yes).

@rudigiesler
Copy link
Author

@SteveBarnett Not at the moment. It's an exact match for now. We could do that, but it would make things a little more complicated (we would have to have different things for different field types, eg. msisdn we would have to look for +, but twitter_id we would have to look for @)

@rudigiesler
Copy link
Author

I think the tests are failing due to the riak version, checking up on this.

@SteveBarnett
Copy link

@rudigiesler How about a shorter / simpler version: strip leading +s from msisdns and leading @s from twitter ids (strip from the thing that is thrown into the query, not from the field value itself, I mean)? Or do we do that already?

@jerith
Copy link
Member

jerith commented Feb 6, 2015

@SteveBarnett This does a Riak index query under the hood, so we can only look up exact values. We could manipulate the query string before we search, but we don't necessarily have + at the front of all MSISDNs, etc.

raise CollectionUsageError(
"Query must be of the form 'field=value'")
valid_keys = self.delivery_classes.keys()
if field not in valid_keys:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could combine these two lines into if field not in self.delivery_classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

And just use self.delivery_classes.keys() in the error message (perhaps sorted?).

@hodgestar
Copy link
Contributor

@SteveBarnett @jerith @rudigiesler contact_field_for_addr which is called by contact_for_addr already ensures that msisdn values have a + on the left and does some sanity adjustments for Gtalk addresses.

@hodgestar
Copy link
Contributor

Slightly radical suggestion -- long term we're trying to get rid of DELIVERY_CLASS in favour of address_types and the delivery_class isn't really relevant here -- we just want to look up a contact given a value for one of it's fields. I propose that we tweak the methods on the contact model in Vumi Go so that there is an underlying method contact_for_addr_field(field, value) that is called by the current contact_for_addr. We can then call that method here too and just keep a list of field names (maybe the field list could be a new tuple attribute on the Contact model).

@rudi Thoughts?

@rudigiesler
Copy link
Author

@hodgestar: It is something we can do, but it will add a bunch of time to this already unplanned task. I'll leave the decision up to you as whether it is worth the extra time or not.

@hodgestar
Copy link
Contributor

@rudi I think let's do this -- it's a little extra work, but it helps kill delivery classes, which is a worthy goal. (copied from Slack for the record).

@rudigiesler
Copy link
Author

@hodgestar This is now using the new vumi-go things. Ready for review.

sorted(Contact.ADDRESS_FIELDS))

try:
value = normalize_addr(field, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the call to normalize_addr be moved outside the try: block?

@hodgestar
Copy link
Contributor

👍 as soon as Travis is happy.

rudigiesler pushed a commit that referenced this pull request Feb 9, 2015
…tact-by-field

Add getting contact by field
@rudigiesler rudigiesler merged commit e30c634 into develop Feb 9, 2015
@rudigiesler rudigiesler deleted the feature/issue-29-add-getting-contact-by-field branch February 9, 2015 12:23
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

4 participants