Adding support for new user-change events with types #1448

filmaj · 2022-05-10T16:14:50Z

These are UserHuddleChangedEvent, UserProfileChangedEvent, UserStatusChangedEvent.

I assume this is a minor version bump but maybe not? Not sure if a new type counts as an API addition? 🤔

…edEvent, UserProfileChangedEvent, UserStatusChangedEvent).

codecov · 2022-05-10T16:17:03Z

Codecov Report

Merging #1448 (832850a) into main (0c996d0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1448   +/-   ##
=======================================
  Coverage   81.97%   81.97%           
=======================================
  Files          18       18           
  Lines        1476     1476           
  Branches      434      434           
=======================================
  Hits         1210     1210           
  Misses        172      172           
  Partials       94       94

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c996d0...832850a. Read the comment docs.

mwbrooks

Looks good to me, but I'll let @srajiang and @seratch have a final approval.

mwbrooks · 2022-05-10T16:56:41Z

src/types/events/base-events.ts

@@ -937,6 +940,240 @@ export interface UserChangeEvent {
  cache_ts: number;
 }

+export interface UserHuddleChangedEvent {


@filmaj for my own reference, what source did you use for the schema reference? api.slack.com, Java/Python, or something else?

api.slack.com, I checked the public-facing docs (i.e. https://api.slack.com/events/user_huddle_changed), and all three event descriptions say:

The event is identical to the existing user_change event. Both user_change and user_huddle_changed are dispatched. at the exact same time.

... so I just copy/pasted the existing user_change event.

Sort of awkward that the verb tense goes from present tense (user_change) to past tense (user_huddle_changed)

There's a lot to be desired with respect to the accuracy / completeness of the api site and I don't know how to make it better. We don't seem to even have a good public facing SOT for user_change to base off of since the example is incomplete so, "everything is identical to user_change" feels unhelpful to me personally.

mwbrooks · 2022-05-10T16:58:25Z

I assume this is a minor version bump but maybe not? Not sure if a new type counts as an API addition? 🤔

I'd also consider this a .minor because we're adding support for the event types - a feature that didn't exist previously. But I'm also curious how we've versioned these changes in the past!

filmaj · 2022-05-10T17:09:26Z

But I'm also curious how we've versioned these changes in the past!

I looked to @srajiang's past PR as a guide before I tackled this: #1008 - it looks like that was a minor bump back then.

srajiang

👋 Did a first pass! I wonder whether we don't get more feedback on the accuracy of these types from the community because people don't use them. Yes this one should probably be a minor.

srajiang · 2022-05-10T17:50:58Z

src/types/events/base-events.ts

+    two_factor_type: string;
+    has_files: boolean;
+    is_workflow_bot: boolean;
+    who_can_share_contact_card: boolean;


Looks like this is at least type string if not constrained further given"EVERYONE" in the sample included on the API site and it applies to user_huddle_changed and user_profile_changed. Seems like this might mean that user_change modeled above also needs a tweak on this field.

srajiang · 2022-05-10T17:58:24Z

src/types/events/base-events.ts

+    is_invited_user?: boolean;
+    has_2fa?: boolean;
+    locale: string;
+    presence: string;


Should this be optional? Not seeing it in the user_huddle_changed sample event but the "this is the exact same thing as user_change` is tripping me up.

srajiang · 2022-05-10T17:59:35Z

src/types/events/base-events.ts

+      is_owner: boolean;
+      teams: string[];
+    };
+    two_factor_type: string;


Seems like if has_2fa is an optional field, two_factor_type should be optional as well. I also don't see this in the event sample on the docs site

srajiang · 2022-05-10T18:00:26Z

src/types/events/base-events.ts

+      teams: string[];
+    };
+    two_factor_type: string;
+    has_files: boolean;


Same comment as above

srajiang · 2022-05-10T18:00:56Z

src/types/events/base-events.ts

+    };
+    two_factor_type: string;
+    has_files: boolean;
+    is_workflow_bot: boolean;


Same comment as above

srajiang · 2022-05-10T18:01:12Z

src/types/events/base-events.ts

+    two_factor_type: string;
+    has_files: boolean;
+    is_workflow_bot: boolean;
+    who_can_share_contact_card: boolean;


Suggested change

who_can_share_contact_card: boolean;

who_can_share_contact_card: string;

srajiang · 2022-05-10T18:05:06Z

src/types/events/base-events.ts

@@ -937,6 +940,240 @@ export interface UserChangeEvent {
  cache_ts: number;
 }

+export interface UserHuddleChangedEvent {


There's a lot to be desired with respect to the accuracy / completeness of the api site and I don't know how to make it better. We don't seem to even have a good public facing SOT for user_change to base off of since the example is incomplete so, "everything is identical to user_change" feels unhelpful to me personally.

filmaj · 2022-05-10T18:56:54Z

Yeah... wow @srajiang I am now testing these events with an app and there are.. a lot of differences. I will try to address them in forthcoming commits as best I can...

filmaj · 2022-05-10T19:19:06Z

src/types/events/base-events.ts

@@ -920,7 +926,7 @@ export interface UserChangeEvent {
    is_invited_user?: boolean;
    has_2fa?: boolean;
    locale: string;
-    presence: string;
+    presence?: string;


FWIW I never saw this field in my testing of these three events

filmaj · 2022-05-10T19:24:52Z

src/types/events/base-events.ts

      first_name: string;
      last_name: string;
-      email: string;
+      email?: string;


Didn't see this field either, even though the "is_email_confirmed" field WAS present and it said "true" 🤔

seratch · 2022-05-10T22:21:59Z

src/types/events/base-events.ts

@@ -882,11 +885,14 @@ export interface UserChangeEvent {
      status_text: string;
      status_text_canonical: string;
      status_emoji: string;
+      status_emoji_display_info: [];


Can you change this empty array type to be the correct one? Same for other events. https://github.com/slackapi/node-slack-sdk/blob/%40slack/web-api%406.7.1/packages/web-api/src/response/UsersInfoResponse.ts#L87

Thanks for catching that @seratch ! I think I have fixed that now.

…user change events

Adding support for new user-change events with types (UserHuddleChang…

921db48

…edEvent, UserProfileChangedEvent, UserStatusChangedEvent).

filmaj added enhancement M-T: A feature request for new functionality semver:minor labels May 10, 2022

filmaj added this to the 3.12.0 milestone May 10, 2022

filmaj requested review from seratch and srajiang May 10, 2022 16:14

filmaj self-assigned this May 10, 2022

mwbrooks reviewed May 10, 2022

View reviewed changes

srajiang requested changes May 10, 2022

View reviewed changes

Tweaking the user change event types based on local testing.

a85bb4b

filmaj requested a review from srajiang May 10, 2022 19:15

filmaj commented May 10, 2022

View reviewed changes

seratch requested changes May 10, 2022

View reviewed changes

Add new StatusEmojiDisplayInfo interface and use that in the various …

832850a

…user change events

filmaj requested a review from seratch May 11, 2022 16:29

seratch approved these changes May 13, 2022

View reviewed changes

seratch merged commit 50f4d34 into main May 13, 2022

seratch deleted the new-user-change-events branch May 13, 2022 01:34

seratch modified the milestones: 3.12.0, 3.11.1 May 13, 2022

seratch added the TypeScript-specific label May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for new user-change events with types #1448

Adding support for new user-change events with types #1448

filmaj commented May 10, 2022

codecov bot commented May 10, 2022 •

edited

Loading

mwbrooks left a comment

mwbrooks May 10, 2022

filmaj May 10, 2022 •

edited

Loading

srajiang May 10, 2022

mwbrooks commented May 10, 2022

filmaj commented May 10, 2022

srajiang left a comment •

edited

Loading

srajiang May 10, 2022 •

edited

Loading

srajiang May 10, 2022

srajiang May 10, 2022

srajiang May 10, 2022

srajiang May 10, 2022

srajiang May 10, 2022

srajiang May 10, 2022

filmaj commented May 10, 2022

filmaj May 10, 2022

filmaj May 10, 2022

seratch May 10, 2022

filmaj May 11, 2022

	who_can_share_contact_card: boolean;
	who_can_share_contact_card: string;

Adding support for new user-change events with types #1448

Adding support for new user-change events with types #1448

Conversation

filmaj commented May 10, 2022

codecov bot commented May 10, 2022 • edited Loading

Codecov Report

mwbrooks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwbrooks commented May 10, 2022

filmaj commented May 10, 2022

srajiang left a comment • edited Loading

Choose a reason for hiding this comment

srajiang May 10, 2022 • 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

filmaj commented May 10, 2022

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

codecov bot commented May 10, 2022 •

edited

Loading

filmaj May 10, 2022 •

edited

Loading

srajiang left a comment •

edited

Loading

srajiang May 10, 2022 •

edited

Loading