Skip to content
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-209] Update CheckWithTech Slack message even if requestor doesn't have Slack linked #193

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

markspolakovs
Copy link
Member

@markspolakovs markspolakovs commented Nov 26, 2024

This pull request includes updates to the handleSlackViewEvent and _sendCWTFollowUpAndUpdateMessage functions in the features/calendar/check_with_tech_actions.ts file to improve error handling and message updating logic.

Error handling improvements:

  • Added a try-catch block around the Slack message update to log errors and ensure the requestor still receives a DM even if the update fails. [1] [2]

Message updating logic enhancements:

  • Moved the logic for finding the requestor's Slack identity and sending a DM to the requestor to after the channel message update. [1] [2]
  • Added new status checks for "Requested" and unknown statuses to ensure proper handling and logging of unexpected statuses.

Other small changes:

  • Added a check for reqType not being "Note" before sending a follow-up message.
  • Cleaned up the function by closing it properly.

@markspolakovs markspolakovs requested review from archessmn and Copilot and removed request for archessmn November 26, 2024 22:18

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (2)

features/calendar/check_with_tech_actions.ts:455

  • The variable 'newContext' is declared but never used. This might be an oversight and should be removed or utilized.
let newContext;

features/calendar/check_with_tech_actions.ts:508

  • [nitpick] The error message 'Failed to update #check-with-tech message' is too generic. Consider providing more context to help with debugging.
console.error("Failed to update #check-with-tech message");
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 suggestion.

Comments skipped due to low confidence (3)

features/calendar/check_with_tech_actions.ts:465

  • [nitpick] The error message 'CWTFollowUp: Expected status other than Requested' could be more descriptive. Consider providing additional context or guidance.
case "Requested":

features/calendar/check_with_tech_actions.ts:467

  • [nitpick] The error message 'CWTFollowUp: Unknown status ' + newStatus could be more descriptive. Consider providing additional context or guidance.
default:

features/calendar/check_with_tech_actions.ts:519

  • The 'requestor' variable is being redefined after the try-catch block. Ensure this redefinition is intentional and doesn't cause any issues.
const requestor = cwt.submitted_by_user.identities.find(

features/calendar/check_with_tech_actions.ts Show resolved Hide resolved
@jenkins-ystv
Copy link

jenkins-ystv bot commented Nov 26, 2024

Deployed a preview of this PR to https://pr-193-internal.dev.ystv.co.uk

@archessmn archessmn merged commit 953b09c into main Dec 3, 2024
3 checks passed
@archessmn archessmn deleted the int-209-cwt-approval-not-synced-back-to-slack branch December 3, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants