Private unfurl user settings #528

wilhelmklopp · 2018-04-13T18:21:10Z

Implements user settings to move us closer to shipping private unfurls

Things to do:

Post message when GitHub account is not linked Private unfurls: Prompt users to connect their GitHub account #536
Add tests
refactor unfurl action routing
move new messages to lib/messages
Implement improvement suggestions by @jmilas
- Cancel -> 'Always ask me'
- Only post 'too many prompts' prompt every 5 times

Deploy notes:

Need to deploy to migrations app first when promoting

…tings

codecov · 2018-04-13T18:29:22Z

Codecov Report

Merging #528 into master will increase coverage by 0.09%.
The diff coverage is 99.13%.

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   98.44%   98.53%   +0.09%     
==========================================
  Files         105      113       +8     
  Lines        1608     1707      +99     
  Branches      201      207       +6     
==========================================
+ Hits         1583     1682      +99     
  Misses         22       22              
  Partials        3        3

…tings

…lhelm-test into private-unfurls-settings

bkeepers

Nice work! This is looking really good.

bkeepers · 2018-04-20T18:24:25Z

lib/middleware/action-attach-metadata.js

+  });
+
+  next();
+};


This looks really similar to the authenticate middleware. Is there any way we could share them? Maybe one to look up the current workspace/user, and the other just verifies that the current user has a GitHub user associated?

Also I'm not sure what action-attach-metadata means in the context of authentication/authorization.

Two ways in which this is different from the authenticate middleware:

(the big one): The authenticate middleware ensures that a GitHub account is connected (to an existing slackUser). action-attach-metadata goes as far as creating a new slackUser if one doesn't exist, just to make the slackUser available on res.locals

The data format is slightly different between slash commands (where authenticate is in use thus far) and actions. For example on slash commands a user id is req.body.user_id, whereas on actions it's req.body.user.id

Also I'm not sure what action-attach-metadata means in the context of authentication/authorization.

I'm not sure if what the middleware does is really authentication/authorization. It's kind of just a helper that makes the model available so that each "action route" doesn't have to worry about it.

Maybe one to look up the current workspace/user, and the other just verifies that the current user has a GitHub user associated?

I agree this would be good to do 👍

bkeepers · 2018-04-20T18:29:34Z

lib/middleware/route-action.js

+  if (res.locals.action === 'cancel') {
+    return res.send({
+      delete_original: true,
+    });


Will this always be the case? Curious if it'd make sense to include this as a middleware/route handler at the end of all the actions that only does this if nothing else has handled the response. 🤷‍♂️

Agreed. Moving this all the way to the end is better, although it does add some additional code to each action handler 🤔

As part of the action based routing, this actually ended up being quite nice

bkeepers · 2018-04-20T18:31:51Z

lib/unfurls/index.js

-  unfurlAction,
+  showRichPreview,
+  unfurlAutoSetting,
+  mutePrompts,


this file is getting a little gnarly. We don't have to anything now, but it'd be good to start thinking about refactoring this to be a little easier to grok.

Moved a lot of things out of this file as part of the action-based routing refactor.

bkeepers · 2018-04-25T21:16:59Z

lib/models/slack-user.js

+        return true;
+      }
+      return false;
+    },


This can be simplified quite a bit:

return this.settings.muteUnfurlPromptsIndefinitely || ( this.settings.muteUnfurlPromptsUntil && moment.unix(this.settings.muteUnfurlPromptsUntil) > moment() )

bkeepers · 2018-04-25T21:19:12Z

lib/slack/index.js

+  router.post('/actions:unfurl-mute-prompts', unfurls.mutePrompts);
+
+  router.post(/actions:.*/, (req) => {
+    throw new Error(`Callback id "${req.body.callback_id}" does not match any patterns`);


…does not match any patterns

nit: I get what that is trying to say, but maybe "not found" would be a better way of describing it?

bkeepers · 2018-04-27T18:56:54Z

lib/unfurls/index.js

@@ -71,16 +78,17 @@ async function linkShared(req, res) {
          user: event.user,
          attachments: [new PrivateUnfurlPrompt(newUnfurl).getAttachment()],
        });
+      } else {
+        return Promise.resolve();


Unless I'm missing something, I think this can just return; without the Promise.resolve()

bkeepers · 2018-04-27T18:58:43Z

lib/unfurls/index.js

+      settings: {
+        muteUnfurlPromptsUntil: moment().add(24, 'hours').unix(),
+      },
+    });


Can this be added to a method on the model?

bkeepers · 2018-04-27T18:59:33Z

lib/unfurls/index.js

+      attachments: [new MutePromptsSettingConfirm('for 24h').getAttachment()],
+    });
+  }
+  if (res.locals.action === 'mute-indefinitely') {


There's quite a few conditionals here. Would it make sense to also try to route based on action?

…option

bkeepers

👍 looks great.

bkeepers · 2018-05-01T18:56:34Z

lib/messages/unfurls/auto-unfurl-prompt.js

@@ -34,7 +34,7 @@ module.exports = class AutoUnfurlPrompt extends Message {
        },
        {
          name: 'cancel',
-          text: 'Cancel',
+          text: 'Always prompt me',


"Always ask me"?

bkeepers · 2018-05-01T18:56:49Z

lib/messages/unfurls/mute-prompts-prompt.js

@@ -29,7 +29,7 @@ module.exports = class MutePromptsPrompt extends Message {
        },
        {
          name: 'cancel',
-          text: 'Cancel',
+          text: 'Always prompt me',


"Always ask me"?

…ack into private-unfurls-settings

Wilhelm Klopp added 5 commits April 9, 2018 14:28

follow up

c6772bc

User settings

2ce311e

Merge remote-tracking branch 'origin/master' into private-unfurls-set…

fe95ecc

…tings

fix linting errors

2bde11d

Remove this model which doesn't exist

89bfad8

Wilhelm Klopp and others added 4 commits April 16, 2018 23:10

Update options to tie it more to the channel

9073a40

Merge branch 'master' into private-unfurls-settings

5c59e8b

Implement muting

02c860c

Refactor unfurl actions to make them more DRY

0c6e14d

wilhelmklopp mentioned this pull request Apr 17, 2018

Private unfurls: Prompt users to connect their GitHub account #536

Merged

Merge branch 'master' into private-unfurls-settings

f2065a4

bkeepers mentioned this pull request Apr 18, 2018

Unfurl: private links #52

Closed

6 tasks

Wilhelm Klopp added 16 commits April 18, 2018 13:17

add auto-unfurl-setting-confirm

548e53e

Add AutoUnfurlPrompt

9316dd8

Add MutePromptsPrompt

901200d

MutePromptsSettingConfirm

28a46e4

Update existing tests

49f2324

Add setup for these test cases

36a0b84

add test for confirmation message after user clicks 'this channel'

9e45448

Add tests for "Enable for this channel" flow

e3ae52d

Add test for dismiss

f129b81

Add test case for when user clicks 'Mute for 24h'

b8918e9

Move location of test

c660293

Move a test to a better place

07d50cf

Add tests for "Mute prompts for 24h" flow

ff9e87d

Add tests for "Mute prompts indefinitely" flow

10848ac

remove only

86e4207

Merge remote-tracking branch 'origin/master' into private-unfurls-set…

d5b0734

…tings

Wilhelm Klopp and others added 3 commits April 20, 2018 17:23

Merge branch 'private-unfurls-settings' of github.com:github-slack/wi…

5f49c58

…lhelm-test into private-unfurls-settings

Add additional test

9a9ad3a

Merge branch 'master' into private-unfurls-settings

ac9982b

wilhelmklopp mentioned this pull request Apr 24, 2018

Open Issue from slack #554

Merged

2 tasks

Merge branch 'master' into private-unfurls-settings

c2c1e28

bkeepers suggested changes Apr 27, 2018

View reviewed changes

wilhelmklopp added 9 commits April 30, 2018 12:40

Make it explicit that this is for private links and clarify 'cancel' …

042013b

…option

Add 'Always prompt me' to mute-prompts-prompt

8d4f74f

Silence loud test

1dd7735

Show dismiss prompt only every 5 times a user clicks 'Dimiss'

1deb2ad

Fix nit

fe392e9

remove promise.resolve()

8db3285

Move more functionality to model

0366488

Refactor unfurl actions

4f64b0b

simplify hasnUnfurlPromptsMuted

fab9c86

bkeepers approved these changes May 1, 2018

View reviewed changes

wilhelmklopp and others added 8 commits May 1, 2018 15:23

Merge branch 'master' into private-unfurls-settings

fe04e5b

Move to 'Always ask me'

5bf10c8

Merge branch 'private-unfurls-settings' of github.com:integrations/sl…

b144676

…ack into private-unfurls-settings

Update tests and snappydooooo

d6126d8

Update description

a5cf09d

Update outdated comments

73ac4a4

Tiny comment improvement

42663df

Add explanatory comment

ef6ed59

wilhelmklopp deployed to staging May 1, 2018 23:01 Active

wilhelmklopp had a problem deploying to production May 1, 2018 23:25 Failure

wilhelmklopp deployed to production May 1, 2018 23:29 Active

wilhelmklopp merged commit 6024b6a into master May 1, 2018

wilhelmklopp deleted the private-unfurls-settings branch May 17, 2018 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Private unfurl user settings #528

Private unfurl user settings #528

wilhelmklopp commented Apr 13, 2018 •

edited

Loading

codecov bot commented Apr 13, 2018 •

edited

Loading

bkeepers left a comment

bkeepers Apr 20, 2018

wilhelmklopp Apr 30, 2018 •

edited

Loading

wilhelmklopp Apr 30, 2018

bkeepers Apr 20, 2018

wilhelmklopp Apr 30, 2018 •

edited

Loading

wilhelmklopp May 1, 2018

bkeepers Apr 20, 2018

wilhelmklopp May 1, 2018

bkeepers Apr 25, 2018

bkeepers Apr 25, 2018

bkeepers Apr 27, 2018

bkeepers Apr 27, 2018

bkeepers Apr 27, 2018

bkeepers left a comment

bkeepers May 1, 2018

bkeepers May 1, 2018

+                });
+                next();
+              };

Private unfurl user settings #528

Private unfurl user settings #528

Conversation

wilhelmklopp commented Apr 13, 2018 • edited Loading

codecov bot commented Apr 13, 2018 • edited Loading

Codecov Report

bkeepers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhelmklopp Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhelmklopp Apr 30, 2018 • edited Loading

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

bkeepers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhelmklopp commented Apr 13, 2018 •

edited

Loading

codecov bot commented Apr 13, 2018 •

edited

Loading

wilhelmklopp Apr 30, 2018 •

edited

Loading

wilhelmklopp Apr 30, 2018 •

edited

Loading