Add external_account type custom profile field. #12442

YJDave · 2019-05-30T08:31:01Z

YJDave · 2019-05-30T08:31:22Z

@timabbott can you review the initial design of model?

timabbott · 2019-05-30T19:03:53Z

zerver/lib/validator.py

+
+    if val.count('%(username)s') != 1:
+        return _('username should appear exactly once in pattern.')
+    val.replace('(username)', '')


Conceptually, I think we should replace it with just the string "username", in case the URL has a structure where a /%(username)s/ -> // would be weird. (I don't think this is a super likely corner case, but it's more obviously correct that way).

timabbott · 2019-05-30T19:13:40Z

@YJDave awesome, thanks for working on this! A few notes:

What is enabled_in_realm for? I don't understand under what situation we'd set that to be False, and am guessing we don't need it.
What I had been imagining for "standard" tools like GitHub/Twitter/etc. is that we'd have either (1) a "subtype" of "Twitter", and no url_pattern in the model (because the url_pattern is completely standard) or (2) a subtype of "Custom" and a url_pattern. And then we'd have a file like zerver/lib/external_accounts.py that defines the standard subtypes (and that data would be included in page_params so that "organization" settings has what it needs to list the options). E.g.

external_accounts = {
    'twitter': {
        'name': 'Twitter',
        'url_pattern': 'https://twitter.com/%(username)s'
    }
}

As a sidenote, we could have the validators for these do an HTTP HEAD request to check if the URL actually works for standard account types, in addition to the validation we have here. May not be something we can do for the Custom subtype (in that the account could be an internal account in some authentication system the Zulip server doesn't have access to).

YJDave · 2019-06-07T05:18:44Z

@timabbott thanks for the detail feedback. I have updated PR.
I will be adding validation on user values(validators for these do an HTTP HEAD request to check if the URL actually works for standard account types).

YJDave · 2019-06-07T05:42:45Z

Let me know changes, Demo:
cc @rishig

What is enabled_in_realm for? I don't understand under what situation we'd set that to be False, and am guessing we don't need it.

I was imagining adding Twitter, GitHub by default in modal instead of hardcode in file.

timabbott · 2019-06-07T05:43:10Z

I merged the preparatory refactor commits, since those looked good. It's failing CI, with an error that looks like one is trying to send more complex objects to e.g. memcached than one should.

+DEFAULT_EXTERNAL_ACCOUNTS = {
+    "twitter": {
+        "text": "Twitter",
+        "url_pattern": "https://twitter.com/%(username)s"
+    },
+    "github": {
+        "text": 'GitHub',
+        "url_pattern": "https://github.com/%(username)s"
+    },
+}

Reading this, maybe we want to s/text/label/ or maybe `s/text/name/``? I feel like that's clearer about the meaning.

timabbott · 2019-06-07T05:43:51Z

zerver/lib/validator.py

+
+    if val.count('%(username)s') != 1:
+        return _('username should appear exactly once in pattern.')
+    val.replace('username', '')


This should be val.replace("%(username)s, "username"), I think

Also, I think you want to make a new variable name -- otherwise you're potentially mutating the argument, which you don't want.

timabbott · 2019-06-07T05:45:22Z

zerver/views/custom_profile_fields.py

-    if error:
-        return json_error(error)
-
+    validate_custom_field_data(field_type, field_data)


I think you can split this refactor into a prep commit to shrink the main commit a bit.

timabbott · 2019-06-07T05:48:33Z

I was imagining adding Twitter, GitHub by default in modal instead of hardcode in file.

I see. I think the way we'd do that sort of thing if we did it is to have new realms have CustomProfileField objects created pointed at those. Let's just leave that off for now -- we should get the main feature working and then we can think about the policy decisions on whether we want to default-include any usernames here.

YJDave · 2019-06-07T05:53:51Z

It's failing CI, with an error that looks like one is trying to send more complex objects to e.g. memcached than one should.

I will go through the documentation and fix soon.

Reading this, maybe we want to s/text/label/ or maybe `s/text/name/``? I feel like that's clearer about the meaning.

You mean, following?

DEFAULT_EXTERNAL_ACCOUNTS = {
    "twitter": {
        "text": {
            "label": "Twitter",
            "name": "twitter",
        },
        "url_pattern": "https://twitter.com/%(username)s"
}

We have dictionary key as value, so not sure we require name. And in case of Custom external accounts, admin add label value as property of CustomProfileField -> name.

YJDave · 2019-06-10T18:13:36Z

@timabbott PR is ready for second review. I have registered realm_default_external_accounts via /register event as you suggested and refactor PR into more commits.

It's failing CI, with an error that looks like one is trying to send more complex objects to e.g. memcached than one should.

Should we dump the data then and parse in frontend?

ujson.dumps(DEFAULT_EXTERNAL_ACCOUNT)

YJDave · 2019-06-17T18:16:09Z

@timabbott I solved conflicts and parse the data. But it still shows the same error, which is not related to the changes 🤔

timabbott · 2019-06-17T22:04:37Z

I'm pretty sure those errors are caused by your changes, just in some subtle way, since they consistently reproduce on this branch. I merged the first couple commits; I'll try to debug a bit.

timabbott · 2019-06-17T22:05:42Z

Here's a simpler test failure I figured out from running test-backend --processes=1:

(zulip-py3-venv) tabbott@coset:~/zulip$ test-backend zerver.tests.test_users.PermissionTest
-- Running tests in serial mode.
Destroying test database for alias 'default'...
Using existing clone for alias 'default'...
Running zerver.tests.test_users.PermissionTest.test_access_user_by_id
Running zerver.tests.test_users.PermissionTest.test_admin_api
Running zerver.tests.test_users.PermissionTest.test_admin_api_hide_emails
Running zerver.tests.test_users.PermissionTest.test_admin_cannot_set_full_name_with_invalid_characters
Running zerver.tests.test_users.PermissionTest.test_admin_cannot_set_long_full_name
Running zerver.tests.test_users.PermissionTest.test_admin_cannot_set_short_full_name
Running zerver.tests.test_users.PermissionTest.test_admin_user_can_change_full_name
Running zerver.tests.test_users.PermissionTest.test_admin_user_can_change_profile_data

======================================================================
ERROR: test_admin_user_can_change_profile_data (zerver.tests.test_users.PermissionTest) (field_name='GitHub')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tabbott/zulip/zerver/tests/test_users.py", line 430, in test_admin_user_can_change_profile_data
    self.assertEqual(field_dict['value'], fields[field_dict['name']])  # type: ignore # Reason in comment
KeyError: 'GitHub'

----------------------------------------------------------------------
Ran 8 tests in 0.490s

FAILED (errors=1)
Destroying test database for alias 'default'...

(I think the exception you were seeing is an error from the parallel sub-process with the failure trying to send details on the failure to the parent process to print).

timabbott · 2019-06-17T22:14:57Z

OK and the bug there is you added a new field type to populate_db without updating a test that relied on that date. Shouldn't be too hard to fix, though we should probably do a prep commit to change the existing "GitHub profile" field (intended to test the "URL" field type) to be instead e.g. "Favorite website" and link to the Zulip ReadTheDocs or something, to avoid confusion among folks using the development environment about why we have two semi-duplicate fields.

YJDave · 2019-06-28T09:11:26Z

@timabbot Thanks for debugging, Updated!
Fixed one style issue with URL type field, which is introduced on my refactorization of custom profile field functions.

Also updated style for non editable input pills.
I checked style on both uses, 1) user groups and 2) user popover,
In user groups, it already have white background.
In user popover:
Before:

AFTER:

YJDave · 2019-06-28T09:15:20Z

static/js/settings_profile_fields.js

    var opts = {
        success_continuation: clear_form_data,
    };
+    field_data = read_field_data_from_form(parseInt(field_type, 10), $('.new-profile-field-form'));


I changed the .new-profile-field-form element here to not fetch from e because there will be only one element anyway in form. It should not create any race condition or other errors. It should work, I tested it manually as well as in casper tests.

Rename URL type custom profile field in populate db to avoid confusion with custom external account profile fields.

Add new custom profile field type, External account. External account field links user's social media profile with account. e.g. GitHub, Twitter, etc. Fixes part of zulip#12302

Type of input element in profile form was not defined for URL type custom fields. Because type was not passed to template.

For non editable input pills, add background color to be transparent and padding to 0px.

YJDave · 2019-07-08T08:55:03Z

Rebased.

timabbott · 2019-07-10T00:48:05Z

This is awesome, merged, thanks @YJDave! There's a few follow-ups we should definitely do:

This effective regression in the visibility of errors: custom profile fields: Show red error indications closer to the fields with validation errors #12748
If you create a new "External account" + "custom type" field, we need to make clear what a correct format looks like. One option would be a placeholder string; the other would be text with an example above the field. @rishig can you make a proposal for how we want this to work? In any case we probably want to make the "URL" format field wider so it's not awkward to enter a typical fairly long URL -- @YJDave can you take care of that?
We should do user docs for this, and maybe advertise on /features and /for/open-source; @rishig want to take this?
We should extend zerver/lib/external_accounts.py with a broader set of reasonable things we want to offer there. skype appears in similar products and we should do GitLab too for GitHub parity; not sure what else makes sense to include. @rishig you should probably be able to just do this if we put together a list since it's just extending a dictionary.

More speculatively, do we want the display for external account fields to be displayed as "[GitHub logo] timabbott", rather than the label-over-text style that we currently use; this would make them significantly more compact, and I think be pretty easy to implement for the built-in ones at least, since we just need to provide the logo URL in the external_accounts.py data. What do you think about this concept?

YJDave · 2019-07-10T20:43:46Z

Thank you! I have submitted followups work. I really like the idea of icon instead of label or maybe with label in profile setting if required. Icon will look really awesome in small user profile. We can add tooltip over icon/logo.

rishig · 2019-07-11T06:36:15Z

External account + custom: Underneath the input box, in grey, maybe:
URL for user zulip123. E.g. https://github.com/zulip123.

Docs updates: sure

Broader set of reasonable things: One question is whether we should include linkedin, which doesn't have usernames but is reasonable to connect to. I think the answer is yes?

Display for external account fields: yeah, I think something like that would be good. I'm trying to think how to combine stuff with usernames (like github) with stuff without (which is maybe just linkedin). Maybe for linkedin we can just put some text like "Visit profile" where there would otherwise be a username?

timabbott · 2019-07-11T19:50:51Z

For the URL, we'd want them to enter https://github.com/%(username)s, so it is important that we have that text in the example.

I think LinkedIn has automatically generated usernames. E.g. I'm https://www.linkedin.com/in/tim-abbott-37828711/. I think we could easily handle this as https://www.linkedin.com/in/%(username)s/, with "help" text that somehow explains how to get it.

timabbott · 2019-07-11T19:51:57Z

Yeah, for LinkedIn we'd just display it as "[logo] LinkedIn" (or "Visit profile"), without a "name".

rishig · 2019-07-11T19:56:52Z

The %(username)s syntax seems unnecessarily complicated?
I guess a part of this recommendation is to change what we accept from "a string that contains %(username)s" to "a string that contains zulip123".

timabbott · 2019-07-11T21:21:57Z

I think zulip123 sounds like an example username, not a thing to be substituted. I think we could switch to https://github.com/{username}; that's a more standard way of expressing variables. But because the variable may not be at the end for a custom system, we kinda need to have a place for it rather than asking for a prefix. The previous one was consistent with linkifiers, but we could reasonably expect to switch the linkifiers syntax to the {username} style (which is also used in our API docs, etc., and also common in basic email form fill tools, so likely to be familiar to many).

rishig · 2019-08-03T16:35:37Z

Switching to https://github.com/{username} would be an improvement (and agreed we should do that for linkifiers as well).

For now, how about:
Must include a %(username)s, e.g. https://twitter.com/%(username)s

We could also add "Email support@zulipchat.com" to the error message for a misformed URL pattern.

zulipbot added the size: XL label May 30, 2019

timabbott reviewed May 30, 2019

View reviewed changes

YJDave force-pushed the cpp_external_account_12302 branch 6 times, most recently from fda23aa to 6e5ce50 Compare June 7, 2019 05:17

zulipbot added the has conflicts label Jun 7, 2019

timabbott reviewed Jun 7, 2019

View reviewed changes

YJDave force-pushed the cpp_external_account_12302 branch 4 times, most recently from ac98153 to e7d7aac Compare June 10, 2019 18:01

YJDave force-pushed the cpp_external_account_12302 branch from e7d7aac to b915436 Compare June 17, 2019 09:30

timabbott force-pushed the cpp_external_account_12302 branch from b915436 to b6ee2ad Compare June 17, 2019 22:04

timabbott mentioned this pull request Jun 17, 2019

Fix handling of test-backend failures with subTest in parallel mode #12596

Open

YJDave force-pushed the cpp_external_account_12302 branch 2 times, most recently from fa4658b to 9cf57f7 Compare June 28, 2019 08:24

YJDave changed the title ~~[WIP] Add external_account type custom profile field.~~ Add external_account type custom profile field. Jun 28, 2019

YJDave force-pushed the cpp_external_account_12302 branch from 9cf57f7 to b2047bc Compare June 28, 2019 08:58

zulipbot removed the has conflicts label Jun 28, 2019

YJDave force-pushed the cpp_external_account_12302 branch from b2047bc to 5c4f337 Compare June 28, 2019 09:10

YJDave commented Jun 28, 2019

View reviewed changes

zulipbot added the has conflicts label Jul 8, 2019

YJDave added 2 commits July 8, 2019 12:19

settings_profile_fields: Refactor function read_field_data_from_form.

6cd0aa3

popluate db: Rename URL type custom profile field.

339b654

Rename URL type custom profile field in populate db to avoid confusion with custom external account profile fields.

YJDave force-pushed the cpp_external_account_12302 branch from 5c4f337 to a9bd668 Compare July 8, 2019 06:55

zulipbot removed the has conflicts label Jul 8, 2019

YJDave added 3 commits July 8, 2019 14:06

models: Add external_account in custom profile field types.

63bb95d

Add new custom profile field type, External account. External account field links user's social media profile with account. e.g. GitHub, Twitter, etc. Fixes part of zulip#12302

user settings: Fix custom URL field style in profile form.

0e6386e

Type of input element in profile form was not defined for URL type custom fields. Because type was not passed to template.

input pills: Update style for non editable pills.

9b84b17

For non editable input pills, add background color to be transparent and padding to 0px.

YJDave force-pushed the cpp_external_account_12302 branch from a9bd668 to 9b84b17 Compare July 8, 2019 08:37

timabbott mentioned this pull request Jul 10, 2019

custom profile fields: Show red error indications closer to the fields with validation errors #12748

Closed

timabbott closed this Jul 10, 2019

YJDave mentioned this pull request Jul 10, 2019

custom fields: Add separate alert-save widget for create field. #12756

Closed

Gittenburg mentioned this pull request Jul 10, 2020

Add support for "External Account" custom profile fields #12302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external_account type custom profile field. #12442

Add external_account type custom profile field. #12442

YJDave commented May 30, 2019

YJDave commented May 30, 2019

timabbott May 30, 2019

timabbott commented May 30, 2019 •

edited

YJDave commented Jun 7, 2019

YJDave commented Jun 7, 2019

timabbott commented Jun 7, 2019

timabbott Jun 7, 2019

timabbott Jun 7, 2019

timabbott Jun 7, 2019

timabbott commented Jun 7, 2019

YJDave commented Jun 7, 2019

YJDave commented Jun 10, 2019

YJDave commented Jun 17, 2019

timabbott commented Jun 17, 2019

timabbott commented Jun 17, 2019

timabbott commented Jun 17, 2019

YJDave commented Jun 28, 2019

YJDave Jun 28, 2019

YJDave commented Jul 8, 2019

timabbott commented Jul 10, 2019

YJDave commented Jul 10, 2019

rishig commented Jul 11, 2019 •

edited

timabbott commented Jul 11, 2019

timabbott commented Jul 11, 2019

rishig commented Jul 11, 2019 •

edited

timabbott commented Jul 11, 2019

rishig commented Aug 3, 2019

Add external_account type custom profile field. #12442

Add external_account type custom profile field. #12442

Conversation

YJDave commented May 30, 2019

YJDave commented May 30, 2019

timabbott May 30, 2019

Choose a reason for hiding this comment

timabbott commented May 30, 2019 • edited

YJDave commented Jun 7, 2019

YJDave commented Jun 7, 2019

timabbott commented Jun 7, 2019

timabbott Jun 7, 2019

Choose a reason for hiding this comment

timabbott Jun 7, 2019

Choose a reason for hiding this comment

timabbott Jun 7, 2019

Choose a reason for hiding this comment

timabbott commented Jun 7, 2019

YJDave commented Jun 7, 2019

YJDave commented Jun 10, 2019

YJDave commented Jun 17, 2019

timabbott commented Jun 17, 2019

timabbott commented Jun 17, 2019

timabbott commented Jun 17, 2019

YJDave commented Jun 28, 2019

YJDave Jun 28, 2019

Choose a reason for hiding this comment

YJDave commented Jul 8, 2019

timabbott commented Jul 10, 2019

YJDave commented Jul 10, 2019

rishig commented Jul 11, 2019 • edited

timabbott commented Jul 11, 2019

timabbott commented Jul 11, 2019

rishig commented Jul 11, 2019 • edited

timabbott commented Jul 11, 2019

rishig commented Aug 3, 2019

timabbott commented May 30, 2019 •

edited

rishig commented Jul 11, 2019 •

edited

rishig commented Jul 11, 2019 •

edited