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

ui/profile: display proper errors in send test dialog #2734

Merged
merged 7 commits into from
Feb 27, 2023

Conversation

cmarquis
Copy link
Contributor

@cmarquis cmarquis commented Dec 8, 2022

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Display proper error messages in the user SendTestDialog

Which issue(s) this PR fixes:
Fixes #2692

Out of Scope:

  • Moved the loader into the dialog rather than it hiding in the bottom of the contact methods list. The dialog now opens immediately and has the loader inside of it.
  • Removed the generic "try again" error message from the dialog in favor of messages from the API.
  • Converted SendTestDialog from Apollo to urql

Screenshots:
Test Delivery Status

Describe any introduced user-facing changes:

  • SendTestDialog now opens immediately after pressing the test button and contains the loader inside of it
  • Proper error messages will be displayed if there is an error attempting to send a test

Additional Info:

  • Now displaying errors from the testContactMethod mutation, if any, in the dialog
  • The notification store now returns an error if a test notification is being sent to a contact method not owned by the current user
  • Updated tests to pass by matching user to contact method
  • Added new tests to verify store functionality

- Now displaying errors from the testContactMethod mutation, if any, in the dialog
- The notification store now returns an error if a test notification is being sent to a contact method not owned by the current user
- Updated tests to pass by matching user to contact method
- Added new tests to verify store functionality
@mastercactapus mastercactapus changed the title Display proper errors in send test dialog ui/profile: display proper errors in send test dialog Dec 12, 2022
johnmarksilly
johnmarksilly previously approved these changes Dec 19, 2022
Copy link
Contributor

@Forfold Forfold left a comment

Choose a reason for hiding this comment

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

LGTM. button isn't disabled, but access denied is returned as an error which I think is fine

Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

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

Fixes the underlying issue, so LGTM.

I think the permission thing can be made better, but that also affects verification, so I'll open a new issue rather than increase the scope.

@mastercactapus mastercactapus merged commit b30dca4 into master Feb 27, 2023
@mastercactapus mastercactapus deleted the 2692-disable-send-test-button branch February 27, 2023 16:14
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.

profile: Send Test shows up for all users
4 participants