-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[INT-201] #CheckWithTech enhancements #168
[INT-201] #CheckWithTech enhancements #168
Conversation
@@ -36,7 +36,7 @@ const envSchema = z.object({ | |||
SLACK_CLIENT_ID: slackEnvType, | |||
SLACK_CLIENT_SECRET: slackEnvType, | |||
SLACK_TEAM_ID: z.string().optional(), | |||
SLACK_CHECK_WITH_TECH_CHANNEL: slackEnvType.default("#check-with-tech"), | |||
SLACK_CHECK_WITH_TECH_CHANNEL: slackEnvType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACHTUNG: there's some places that the slack API requires a channel ID, so I remove the default here. We need to make sure the ID is set in ystv/nomad prior to merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Deployed a preview of this PR to https://pr-168-internal.dev.ystv.co.uk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have these bits changed as well as having the "need help" flow use it as well.
I've made a few UI changes but otherwise looks good.
}, | ||
}); | ||
if (!actor) { | ||
await respond({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have this message only show for the user that clicked it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fairly sure that this should be an ephemeral message, per https://api.slack.com/interactivity/handling#publishing_ephemeral_response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that's what it was but couldn't remember if it was something different for slack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got two accounts on my dev instance and can see the messages on both
return; | ||
} | ||
if (!userHasPermission(actor.user_id, "CheckWithTech.Admin")) { | ||
await respond({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with only showing to the single user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response
@@ -22,6 +22,8 @@ export const PermissionEnum = z.enum([ | |||
"Calendar.Public.Admin", | |||
"Calendar.Public.Creator", | |||
"CalendarIntegration.Admin", | |||
"CheckWithTech.Submit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this permission do, does this stop people submitting requests if they don't have it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And doesn't show the prompt at all - essentially intended as a feature flag.
TODO: