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.

Sign up for GitHub

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

Jump to bottom

refactor: move notification types into content field #2773

Merged
merged 8 commits into from
Sep 18, 2024
Merged

refactor: move notification types into content field #2773

merged 8 commits into from
Sep 18, 2024

Conversation

firestack
Copy link
Member

Asana Ticket: https://app.asana.com/0/0/1208212248665131/f

When looking to implement a new notification type, I ran into an issue where it's hard to deduce on the frontend which fields should be present because the same struct was being shared for multiple notification types. This finishes up the work of having multiple notification types by splitting it up for the frontend to be able to match on.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 5, 2024

Coverage of commit 5cb0a67

Summary coverage rate:
  lines......: 93.0% (3286 of 3534 lines)
  functions..: 72.4% (1359 of 1876 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |95.7%     69|83.3%    18|    -      0

Download coverage report

@firestack firestack marked this pull request as ready for review September 6, 2024 16:58
@firestack firestack requested a review from a team as a code owner September 6, 2024 16:58
@firestack firestack changed the title /refactor: move notification types into content field refactor: move notification types into content field Sep 10, 2024
@firestack firestack force-pushed the kf/refactor/notifications-content-type branch 2 times, most recently from e525e64 to c9df205 Compare September 10, 2024 18:38
@github-actions GitHub Actions
Copy link

Coverage of commit c9df205

Summary coverage rate:
  lines......: 93.0% (3304 of 3554 lines)
  functions..: 72.4% (1362 of 1882 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |95.7%     69|83.3%    18|    -      0

Download coverage report

@github-actions GitHub Actions
Copy link

Coverage of commit c9df205

Summary coverage rate:
  lines......: 93.0% (3305 of 3554 lines)
  functions..: 72.4% (1363 of 1882 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |95.7%     69|83.3%    18|    -      0

Download coverage report

@firestack firestack force-pushed the kf/refactor/notifications-content-type branch from c9df205 to 78c9cf9 Compare September 16, 2024 13:02
@github-actions GitHub Actions
Copy link

Coverage of commit 78c9cf9

Summary coverage rate:
  lines......: 92.6% (3305 of 3570 lines)
  functions..: 71.8% (1363 of 1899 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |95.7%     69|83.3%    18|    -      0

Download coverage report

@firestack firestack force-pushed the kf/refactor/notifications-content-type branch 2 times, most recently from 8c751cb to 4cabc12 Compare September 17, 2024 12:04
@github-actions GitHub Actions
Copy link

Coverage of commit 4cabc12

Summary coverage rate:
  lines......: 92.5% (3302 of 3568 lines)
  functions..: 71.8% (1365 of 1901 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |94.0%     67|85.0%    20|    -      0

Download coverage report

@firestack firestack assigned hannahpurcell and unassigned firestack Sep 17, 2024
@github-actions GitHub Actions
Copy link

Coverage of commit 4cabc12

Summary coverage rate:
  lines......: 92.5% (3302 of 3568 lines)
  functions..: 71.8% (1365 of 1901 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |94.0%     67|85.0%    20|    -      0

Download coverage report

@firestack firestack force-pushed the kf/refactor/notifications-content-type branch from 4cabc12 to 3a8cfc5 Compare September 17, 2024 12:21
@github-actions GitHub Actions
Copy link

Coverage of commit 3a8cfc5

Summary coverage rate:
  lines......: 92.5% (3301 of 3568 lines)
  functions..: 71.8% (1364 of 1901 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/notifications/db/block_waiver.ex                            |66.7%      3|71.4%     7|    -      0
  lib/notifications/db/bridge_movement.ex                         |66.7%      3|71.4%     7|    -      0
  lib/notifications/notification.ex                               | 100%     54|90.0%    10|    -      0
  lib/notifications/notification_server.ex                        |94.0%     67|85.0%    20|    -      0

Download coverage report

hannahpurcell
hannahpurcell approved these changes Sep 18, 2024
View reviewed changes
Copy link
Collaborator

@hannahpurcell hannahpurcell left a comment

Choose a reason for hiding this comment

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

Great!! Thanks for covering all my questions. This was quite the undertaking, so bravo

@firestack firestack merged commit 348e331 into main Sep 18, 2024
33 checks passed
@firestack firestack deleted the kf/refactor/notifications-content-type branch September 18, 2024 15:20
@firestack firestack mentioned this pull request Sep 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshlarson joshlarson joshlarson left review comments

@hannahpurcell hannahpurcell hannahpurcell approved these changes

Assignees

@hannahpurcell hannahpurcell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@firestack @joshlarson @hannahpurcell