Replace AddPhoneNumber with AddDestination #492

podliashanyk · 2023-03-15T12:01:51Z

Changes made:

Clicking on pluss-button in Notification Profiles will prompt a new add destination component (instead of the add phone number component)
All use of phone numbers is removed/replaced (tests, styles, components, types, api calls etc), except of phone numbers as a part of destinations

Closes #481

codecov-commenter · 2023-03-15T12:04:11Z

Codecov Report

Merging #492 (632625a) into master (607d295) will increase coverage by 3.18%.
The diff coverage is 48.48%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   53.89%   57.07%   +3.18%     
==========================================
  Files          85       85              
  Lines        3570     3553      -17     
  Branches      739      744       +5     
==========================================
+ Hits         1924     2028     +104     
+ Misses       1637     1516     -121     
  Partials        9        9

Impacted Files	Coverage Δ
src/api/client.ts	`38.11% <ø> (+1.19%)`	⬆️
src/api/hooks.tsx	`85.10% <ø> (+1.43%)`	⬆️
src/components/destinations/Destination.tsx	`2.85% <ø> (ø)`
...ts/notificationprofile/NotificationProfileList.tsx	`43.26% <36.95%> (-2.74%)`	⬇️
...ts/notificationprofile/NotificationProfileCard.tsx	`90.14% <66.66%> (+0.14%)`	⬆️
src/components/destinations/NewDestination.tsx	`96.82% <83.33%> (+94.34%)`	⬆️
...nents/notificationprofile/AddDestinationDialog.tsx	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

hmpf

I will test it manually now, will approve later.

This PR was mostly noise from formatting changes. Please do not combine this many formatting changes (whitespace, indentation, quote-type, newlines, comma vs. semi-colon vs. nothing etc.) and actual functionality changes in the same commits again.

No need to split this PR up, just don't do it again.

hmpf · 2023-03-16T08:48:57Z

CHANGELOG.md

-## [v1.8.1] - 2023-02-15
-
-### Fixed
-
- Ensure that description is stored properly when acking, both in bulk and directly
-
-## [v1.8.0] - 2023-02-02

 ### Added

 - Use backend bulk endpoints to speed up updating incidents in bulk
- Visual feedback (table loading) when changing incident filter parameters, and when navigating incident table pages
-


Well, changelog needs updating anyway.

hmpf · 2023-03-16T08:49:53Z

NOTES.md

-## [v1.8.0] - 2023-02-15
+### Changed

-### Fixed

- Ensure that description is stored properly when acking, both in bulk and directly


Needs merging in so it is not lost.

This one (and other updates in CHANGELOG.md and NOTES.md) are not removed. They are just moved. This one is f.e moved to line 25.
When updating changelogs I noticed that some of the earlier updates were moved from unreleased to some random release version. So I moved them back to unreleasedin this PR. Should have done it in a separate commit though 😅

hmpf · 2023-03-16T08:50:56Z

src/api/client.ts

- PhoneNumberPK,
- PhoneNumber,
- PhoneNumberRequest,
- PhoneNumberSuccessResponse,


Forgive the snark but this is one reason it is handy to keep imports sorted! Easier to change things.

hmpf · 2023-03-16T08:51:47Z

src/api/client.ts

@@ -209,45 +205,6 @@ class ApiClient {
 );
 }

- // Phone number


Big fan of "abusing" comments this way, again, makes it easier to change things/find things!

hmpf · 2023-03-16T08:53:35Z

src/api/types.d.ts

@@ -122,14 +112,14 @@ export type DestinationPK = number;
 export type Media = {
 slug: string;
 name: string;
-}
+};


Is there a new linter that demands ";"? Does make for noise.

Prettier config 😅 My bad, I have fixed it now so next PRs will not come with tons of code reformats! 🥲

hmpf · 2023-03-16T08:55:53Z

src/api/types.d.ts

+ "server-version": string;
+ "api-version": {
 stable: string;
 unstable: string;
 };
- 'jsonapi-schema': {
+ "jsonapi-schema": {


This file-change is almost all noise. Beh.

Prettier config 😅 My bad, I have fixed it now so next PRs will not come with tons of code reformats! 🥲

hmpf · 2023-03-16T08:56:30Z

src/api/types.d.ts


 export type MediaProperty = {
 title: string;
 type: string; // value type
 description?: string;
 format?: string;
-}
+};

 export enum KnownProperties {
 PHONE_NUMBER = "phone_number",


Is PHONE_NUMBER = "phone_number", still needed?

.. and why does it end with "," and not ";"?

Yes, this is a list of what the properties of the known media is and for destinations of the medium sms it is phone_number.

And the comma is because it is an enum.

Yes, we still have phone numbers as a part of destinations (like in the type you highlighted f.e.), in the same fashion as emails etc. But the notion of freestanding phone number is removed from Argus completely now.

hmpf · 2023-03-16T09:08:40Z

src/components/notificationprofile/NotificationProfileList.test.tsx

-import {
- Destination,
- Filter, Media,
- NotificationProfile,
- PhoneNumber,
- Timeslot


This is the only change in this file that removes the old way to do phone numbers?

Yes, PhoneNumber import. It was used in apiMock to phone number endpoint call before. I changed it to apiMock call to destinations in one of the earlier PRs. So rogue import was the only thing that remained in this file.

johannaengland

Even when I zoom out to 50% the popup still has vertical and horizontal scroll bars.

When creating a destination from the notification profile page I would expect it not to only be added to destinations, but also automatically added to the profile I'm creating it from or is that no ideal?

CHANGELOG.md

NOTES.md

johannaengland · 2023-03-16T11:06:20Z

src/api/types.d.ts


 export type MediaProperty = {
 title: string;
 type: string; // value type
 description?: string;
 format?: string;
-}
+};

 export enum KnownProperties {
 PHONE_NUMBER = "phone_number",


Yes, this is a list of what the properties of the known media is and for destinations of the medium sms it is phone_number.

And the comma is because it is an enum.

podliashanyk · 2023-03-16T14:46:49Z

Even when I zoom out to 50% the popup still has vertical and horizontal scroll bars.

Fixed

When creating a destination from the notification profile page I would expect it not to only be added to destinations, but also automatically added to the profile I'm creating it from or is that no ideal?

I simply mirrored the add phone number functionality in this PR, and this wasn't a part of the feature with phone numbers. New destination is therefore only added to the dropdown in notifications and to destinations.
I suggest creating a separate issue for this. Such changes are out of scope of this PR anyway.
What you suggest feels a bit off for me personally, but user might think otherwise. 😄 So if we choose to work on the issue we should get feedback from the users who work a lot with the notification profiles

podliashanyk · 2023-03-16T14:54:56Z

This PR was mostly noise from formatting changes. Please do not combine this many formatting changes (whitespace, indentation, quote-type, newlines, comma vs. semi-colon vs. nothing etc.) and actual functionality changes in the same commits again.

No need to split this PR up, just don't do it again.

Sorry, my bad! Will keep an eye on it 🤯 I have fixed the formatter config now.

johannaengland

No more unnecessary scrollbars!

And I made an issue for automatically adding a created destination to a profile.

Very nice! 👍

hmpf · 2023-03-17T07:37:11Z

CHANGELOG.md

-
- Ensure that description is stored properly when acking, both in bulk and directly
-
-## [v1.8.0] - 2023-02-02

 ### Added

 - Use backend bulk endpoints to speed up updating incidents in bulk


"Use backend bulk endpoints to speed up updating incidents in bulk" is released isn't it? In the notes, it is sorted under version 1.8.0, not unreleased.

hmpf · 2023-03-17T07:38:19Z

NOTES.md

 - Use backend bulk endpoints to speed up updating incidents in bulk
- Visual feedback (table loading) when changing incident filter parameters, and when navigating incident table pages
+- Replaced _add phone number from notification profiles_ feature with _add destination from notification profiles_ feature.


I saw wrong but: isn't the bulk stuff already in production?

hmpf · 2023-03-17T07:42:18Z

Polish, do not change now: on the "Notification profiles"-page, on Vivaldi (which uses chrome), the lines under the fields does not line up.

hmpf · 2023-03-17T07:43:13Z

@johannaengland's follow up issue: #493

podliashanyk · 2023-03-17T08:40:17Z

@hmpf CHANGELOG and NOTES are fixed now. And I commented on the cause of the problem with line up in #494

hmpf

Ready for rebase and merge.

Disable scrollbars Switch to using destinations only in notifications list

Remove use of phone numbers in styles Pluss some formatting and styling fixes Remove add phone number dialog component Remove phone number from notification list tests Remove tests for add phone number dialog Component no longer exists Replace phone numbers with destinations in card tests

Test reset of input fields in new destination component Test discard button

Update CHANGELOG.md Co-authored-by: Johanna England <[email protected]> Update NOTES.md Co-authored-by: Johanna England <[email protected]> Fix changelogs

Implement add destination dialog component

ce874b7

podliashanyk added notifications Notification system + notification profiles APIv2 Compatibility with APIv2 is guaranteed, may or may not be compatible with other API versions labels Mar 15, 2023

podliashanyk self-assigned this Mar 15, 2023

podliashanyk requested review from hmpf, johannaengland, lunkwill42 and stveit March 15, 2023 13:19

hmpf reviewed Mar 16, 2023

View reviewed changes

johannaengland requested changes Mar 16, 2023

View reviewed changes

podliashanyk requested review from hmpf and johannaengland March 16, 2023 14:46

johannaengland mentioned this pull request Mar 17, 2023

Automatically adding destination to profile if created from it #493

Open

johannaengland approved these changes Mar 17, 2023

View reviewed changes

hmpf requested changes Mar 17, 2023

View reviewed changes

hmpf mentioned this pull request Mar 17, 2023

Polish: Make the lines under the fields on the "Notification profiles"-page line up #494

Closed

podliashanyk requested a review from hmpf March 17, 2023 08:40

hmpf approved these changes Mar 17, 2023

View reviewed changes

podliashanyk added 4 commits March 17, 2023 09:52

Switch to destinations in notification profile card

1b235d8

Disable scrollbars Switch to using destinations only in notifications list

Add tests for add destination dialog component

2487dfb

Test reset of input fields in new destination component Test discard button

Update changelogs

7775f88

Update CHANGELOG.md Co-authored-by: Johanna England <[email protected]> Update NOTES.md Co-authored-by: Johanna England <[email protected]> Fix changelogs

podliashanyk force-pushed the implement-add-new-destination-from-notifications branch from d15db43 to 7775f88 Compare March 17, 2023 08:54

podliashanyk merged commit ec9a621 into master Mar 17, 2023

podliashanyk deleted the implement-add-new-destination-from-notifications branch March 17, 2023 09:00

This was referenced Mar 17, 2023

Support new notificationprofile structure #357

Closed

Add support for multiple email addresses per person #330

Closed

lunkwill42 mentioned this pull request Mar 20, 2023

Let user create destinations in settings #416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace AddPhoneNumber with AddDestination #492

Replace AddPhoneNumber with AddDestination #492

podliashanyk commented Mar 15, 2023

codecov-commenter commented Mar 15, 2023 •

edited

Loading

hmpf left a comment

hmpf Mar 16, 2023

hmpf Mar 16, 2023

podliashanyk Mar 16, 2023

hmpf Mar 16, 2023

hmpf Mar 16, 2023

hmpf Mar 16, 2023

podliashanyk Mar 16, 2023

hmpf Mar 16, 2023

podliashanyk Mar 16, 2023

hmpf Mar 16, 2023

hmpf Mar 16, 2023

johannaengland Mar 16, 2023

podliashanyk Mar 16, 2023

hmpf Mar 16, 2023

podliashanyk Mar 16, 2023

johannaengland left a comment

johannaengland Mar 16, 2023

podliashanyk commented Mar 16, 2023 •

edited

Loading

podliashanyk commented Mar 16, 2023 •

edited

Loading

johannaengland left a comment

hmpf Mar 17, 2023

podliashanyk Mar 17, 2023

hmpf Mar 17, 2023

podliashanyk Mar 17, 2023

hmpf commented Mar 17, 2023

hmpf commented Mar 17, 2023

podliashanyk commented Mar 17, 2023

hmpf left a comment

Replace AddPhoneNumber with AddDestination #492

Replace AddPhoneNumber with AddDestination #492

Conversation

podliashanyk commented Mar 15, 2023

codecov-commenter commented Mar 15, 2023 • edited Loading

Codecov Report

hmpf left a 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

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

johannaengland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

podliashanyk commented Mar 16, 2023 • edited Loading

podliashanyk commented Mar 16, 2023 • edited Loading

johannaengland left a 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

hmpf commented Mar 17, 2023

hmpf commented Mar 17, 2023

podliashanyk commented Mar 17, 2023

hmpf left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 15, 2023 •

edited

Loading

podliashanyk commented Mar 16, 2023 •

edited

Loading

podliashanyk commented Mar 16, 2023 •

edited

Loading