Add URN schemes to variable completions #1067

norkans7 · 2017-01-30T13:39:40Z

No description provided.

ericnewcomer

What is the use case here?

ericnewcomer · 2017-01-30T14:44:24Z

temba/msgs/tests.py

+        # a Twitter channel
+        Channel.create(self.org, self.user, None, 'TT')
+        completions.insert(-2, dict(name="contact.%s" % 'twitter', display="Contact %s" % "Twitter handle"))
+


Why the substitution here?

completions is a list and I want to keep the order right so I inserted before the 2 dict representing the contact fields above

norkans7 · 2017-01-30T14:54:10Z

This is to allow @contact.twitter to be auto completed currently it was not while their values are included in the contact message_context

nicpottier · 2017-01-31T20:57:42Z

temba/contacts/models.py

+
+        if not urn and scheme is not None:
+            return ''
+
        if org.is_anon:
            return self.anon_identifier


What is this doing here? This is returning a function pointer isn't it?

Agree that it would make sense for us to return a generic masked version of the URN here. (IE, just ******** for anon orgs). Though empty string if the contact doesn't have an URN of that scheme.

were are return "000173823" for anon orgs not *******

Ya, that seems wrong to return for @contact.tel don't you think?

Okay I can change this to make sure it is ******** only in the message context(scheme is not None) and leave the other place be 00000173823 for the contacts views

Ah, ya the contact views should maybe be switching higher up on anon orgs.. (ie deciding to show the ID before calling get_urn_display)

we use a template tag that calls get_urn_display

nicpottier · 2017-01-31T21:01:15Z

temba/flows/views.py

@@ -789,7 +790,12 @@ def render_to_response(self, context, **response_kwargs):
                dict(name='contact.uuid', display=six.text_type(_("Contact UUID"))),
                dict(name='new_contact', display=six.text_type(_('New Contact')))
            ]
-            contact_variables += [dict(name="contact.%s" % field.key, display=field.label) for field in ContactField.objects.filter(org=org, is_active=True)]
+
+            contact_variables += [dict(name="contact.%s" % scheme, display="Contact %s" % label) for scheme, label in


…apidpro into add-urn-schemes-to-completions

rowanseymour · 2017-02-01T10:15:06Z

General thought: Contact.get_display and Contact.get_urn_display need to clear up who does what. Currently...

get_display returns the name if its set, otherwise the pk if org is anon, or a URN.
get_urn_display returns a URN of the requested type, or nothing if there isn't one... except if the org is anon if returns the pk regardless of the contact's URNs.

I'm thinking get_urn_display should always return a URN or empty or a masked URN. For any place we want this to sometimes be the pk, that logic should be elsewhere.

rowanseymour · 2017-02-01T09:52:57Z

temba/contacts/models.py

        if org.is_anon:
+            if scheme is not None:
+                return Contact.HIDDEN_URN_MASK


Already have ContactURN.ANON_MASK for this

…emes-to-completions

…apidpro into add-urn-schemes-to-completions

nicpottier · 2017-02-01T15:00:37Z

temba/contacts/models.py

@@ -1736,8 +1736,13 @@ def get_urn_display(self, org=None, scheme=None, formatted=True, international=F
        if not org:
            org = self.org

+        urn = self.get_urn(scheme)
+
+        if not urn and scheme is not None:


What happens if get_urn_display is called with no scheme on an anon org on a contact which has no URNs. Won't it return the ANON_MASK instead of ''?

Yes that is what is expected, we want to have '' only in the contact message context which is where we call the method with the scheme

I don't think that should be as expected.. get_urn_display should only return the ANON_MASK if there is an URN that exists for the contact. You are saying if there is no URN on a contact we'll still get the ANON_MASK instead of ''?

nicpottier · 2017-02-01T15:04:12Z

temba/contacts/templatetags/contacts.py

@@ -66,7 +66,8 @@ def format_urn(urn_or_contact, org):
        return urn_val if urn_val != ContactURN.ANON_MASK else '\u2022' * 8  # replace *'s with prettier HTML entity
    elif isinstance(urn_or_contact, Contact):
        # will render contact's highest priority URN
-        return urn_or_contact.get_urn_display(org=org, international=True)
+        urn_val = urn_or_contact.get_display(org=org)
+        return urn_val if urn_val != ContactURN.ANON_MASK else '\u2022' * 8  # replace *'s with prettier HTML entity


Maybe we should be calling this method something different. format_urn sure sounds like it should return a URN.

This should only accept an URN as an argument, and we should have a filter for format_contact for that case that shows the anon id for anon orgs, otherwise the highest priority URN.

nicpottier · 2017-02-01T15:06:27Z

temba/flows/views.py

@@ -789,7 +790,13 @@ def render_to_response(self, context, **response_kwargs):
                dict(name='contact.uuid', display=six.text_type(_("Contact UUID"))),
                dict(name='new_contact', display=six.text_type(_('New Contact')))
            ]
-            contact_variables += [dict(name="contact.%s" % field.key, display=field.label) for field in ContactField.objects.filter(org=org, is_active=True)]
+
+            contact_variables += [dict(name="contact.%s" % scheme, display=six.text_type(_("Contact %s" % label)))


So this will read "Contact Facebook" or "Contact Twitter" ya? Seems we might want something more like "Contact Facebook URN" and "Contact Twitter URN" instead.. Though from a naming perspective that is slightly problematic since these won't be full URNs but just the paths right?

Here we'll get "Contact Facebook identifier" or "Contact Twitter handle"

as seen on
https://github.com/nyaruka/rapidpro/blob/master/temba/contacts/models.py#L61

Oh ok great.

norkans7 · 2017-02-01T21:03:22Z

@nicpottier this is updated please review

nicpottier · 2017-02-01T21:08:38Z

temba/contacts/models.py

        if org.is_anon:
-            return self.anon_identifier
+            return ContactURN.ANON_MASK

        urn = self.get_urn(scheme)


No need to call this again..

nicpottier · 2017-02-01T21:10:30Z

temba/contacts/templatetags/contacts.py

-        raise ValueError('Must be a URN or contact')
+def format_urn(urn, org):
+    urn_val = urn.get_display(org=org, international=True)
+    return urn_val if urn_val != ContactURN.ANON_MASK else '\u2022' * 8  # replace *'s with prettier HTML entity


Lets not use this kind of operator when we want to comment things, just use a normal if block here.

nicpottier · 2017-02-01T21:10:43Z

temba/flows/views.py

@@ -789,7 +790,13 @@ def render_to_response(self, context, **response_kwargs):
                dict(name='contact.uuid', display=six.text_type(_("Contact UUID"))),
                dict(name='new_contact', display=six.text_type(_('New Contact')))
            ]
-            contact_variables += [dict(name="contact.%s" % field.key, display=field.label) for field in ContactField.objects.filter(org=org, is_active=True)]
+
+            contact_variables += [dict(name="contact.%s" % scheme, display=six.text_type(_("Contact %s" % label)))


Oh ok great.

…apidpro into add-urn-schemes-to-completions

nicpottier · 2017-02-01T21:53:52Z

Doh, coverage!

norkans7 · 2017-02-01T22:18:57Z

that line can never be reached, coverage should be good now

nicpottier

Add URN schemes to variable completions

4770bd5

norkans7 added the review label Jan 30, 2017

fix tests

843eaa0

ericnewcomer reviewed Jan 30, 2017

View reviewed changes

norkans7 added 2 commits January 30, 2017 22:47

tweak get_urn_display to give hint to if no value is in the URN

3d65d3e

Merge branch 'master' into add-urn-schemes-to-completions

9cd1058

norkans7 requested a review from nicpottier January 31, 2017 20:58

norkans7 self-assigned this Jan 31, 2017

nicpottier requested changes Jan 31, 2017

View reviewed changes

norkans7 added 4 commits January 31, 2017 23:33

localize labels

accfad7

use ******* for urn values in contact message context on anon org

6715ca5

Merge branch 'add-urn-schemes-to-completions' of github.com:nyaruka/r…

c0f081a

…apidpro into add-urn-schemes-to-completions

Merge branch 'master' into add-urn-schemes-to-completions

976ea56

norkans7 requested a review from rowanseymour February 1, 2017 09:35

rowanseymour reviewed Feb 1, 2017

View reviewed changes

norkans7 added 4 commits February 1, 2017 13:17

Merge branch 'master' of github.com:nyaruka/rapidpro into add-urn-sch…

df7d878

…emes-to-completions

Adjust format_urn and remove anon_identifier from get_urn_display

7537899

Merge branch 'add-urn-schemes-to-completions' of github.com:nyaruka/r…

d8e9d29

…apidpro into add-urn-schemes-to-completions

use Contact.get_display in format_urn

a719eb8

nicpottier requested changes Feb 1, 2017

View reviewed changes

norkans7 added 2 commits February 1, 2017 22:57

return '' if not URN on the contact, add format_contact filter

1b5fe66

Merge branch 'master' into add-urn-schemes-to-completions

8cb3eb0

nicpottier reviewed Feb 1, 2017

View reviewed changes

nicpottier requested changes Feb 1, 2017

View reviewed changes

norkans7 added 2 commits February 1, 2017 23:30

use normal if

943daaa

Merge branch 'add-urn-schemes-to-completions' of github.com:nyaruka/r…

eab17f6

…apidpro into add-urn-schemes-to-completions

nicpottier approved these changes Feb 1, 2017

View reviewed changes

remove unreachable if case

18b245d

Merge branch 'master' into add-urn-schemes-to-completions

0435ebe

nicpottier approved these changes Feb 1, 2017

View reviewed changes

nicpottier merged commit fb53fd2 into master Feb 1, 2017

nicpottier removed the review label Feb 1, 2017

nicpottier deleted the add-urn-schemes-to-completions branch February 1, 2017 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add URN schemes to variable completions #1067

Add URN schemes to variable completions #1067

norkans7 commented Jan 30, 2017

ericnewcomer left a comment

ericnewcomer Jan 30, 2017

norkans7 Jan 30, 2017

norkans7 commented Jan 30, 2017

nicpottier Jan 31, 2017

nicpottier Jan 31, 2017

norkans7 Jan 31, 2017

nicpottier Jan 31, 2017

norkans7 Jan 31, 2017

nicpottier Jan 31, 2017

norkans7 Jan 31, 2017

nicpottier Jan 31, 2017

rowanseymour commented Feb 1, 2017

rowanseymour Feb 1, 2017

nicpottier Feb 1, 2017

norkans7 Feb 1, 2017

nicpottier Feb 1, 2017

nicpottier Feb 1, 2017

norkans7 Feb 1, 2017

nicpottier Feb 1, 2017

norkans7 Feb 1, 2017

nicpottier Feb 1, 2017

norkans7 commented Feb 1, 2017

nicpottier Feb 1, 2017

nicpottier Feb 1, 2017

nicpottier Feb 1, 2017

nicpottier commented Feb 1, 2017

norkans7 commented Feb 1, 2017

nicpottier left a comment

Add URN schemes to variable completions #1067

Add URN schemes to variable completions #1067

Conversation

norkans7 commented Jan 30, 2017

ericnewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norkans7 commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanseymour commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norkans7 commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicpottier commented Feb 1, 2017

norkans7 commented Feb 1, 2017

nicpottier left a comment

Choose a reason for hiding this comment