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

Notifications Enabled #760

Merged
merged 9 commits into from
Apr 5, 2023
Merged

Conversation

Velin92
Copy link
Member

@Velin92 Velin92 commented Apr 3, 2023

Enabled notifications with default static text.

Also implemented a way to get to the original user that sent the notification (which will be useful for multi account support)

WHAT IS MISSING:

  • Permalink support
  • A way to handle navigations where we set coordinators without animations

notifications enabled with default static text

code improvement

added the share key
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against 3ba8b3e

@Velin92 Velin92 linked an issue Apr 3, 2023 that may be closed by this pull request
@Velin92 Velin92 marked this pull request as ready for review April 3, 2023 17:50
@Velin92 Velin92 requested a review from a team as a code owner April 3, 2023 17:50
@Velin92 Velin92 requested review from stefanceriu and removed request for a team April 3, 2023 17:50
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 39.39% and project coverage change: -0.02 ⚠️

Comparison is base (2439431) 50.82% compared to head (638f2f8) 50.80%.

❗ Current head 638f2f8 differs from pull request most recent head 3ba8b3e. Consider uploading reports for the commit 3ba8b3e to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #760      +/-   ##
===========================================
- Coverage    50.82%   50.80%   -0.02%     
===========================================
  Files          292      292              
  Lines        16072    16010      -62     
  Branches      9762     9740      -22     
===========================================
- Hits          8168     8134      -34     
+ Misses        7664     7637      -27     
+ Partials       240      239       -1     
Flag Coverage Δ
unittests 25.43% <39.39%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ElementX/Sources/Application/AppSettings.swift 95.91% <ø> (ø)
ElementX/Sources/Other/Extensions/Bundle.swift 58.97% <0.00%> (-41.03%) ⬇️
...s/Screens/RoomDetails/View/RoomDetailsScreen.swift 41.50% <0.00%> (ø)
...omMemberDetails/View/RoomMemberDetailsScreen.swift 43.37% <0.00%> (-1.21%) ⬇️
...ces/Notification/Proxy/NotificationItemProxy.swift 0.00% <0.00%> (ø)
.../Notification/Proxy/NotificationServiceProxy.swift 0.00% <0.00%> (ø)
...ces/Notification/Manager/NotificationManager.swift 85.60% <77.77%> (-1.18%) ⬇️
...ources/Services/UserSession/RestorationToken.swift 100.00% <100.00%> (ø)

... and 15 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Looks good to me but I left some changes inline.

Also, today's merge commit should've been a rebase instead.

@Velin92
Copy link
Member Author

Velin92 commented Apr 4, 2023

Looks good to me but I left some changes inline.

Also, today's merge commit should've been a rebase instead.

@stefanceriu actually regarding this... I used the default github update branch option, but this always defaults to a merge, at this point could we just disable it, or there is a way to force it to be always a rebase?

@Velin92 Velin92 requested a review from stefanceriu April 5, 2023 17:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

🚢 it!

@Velin92 Velin92 merged commit 0bce991 into develop Apr 5, 2023
@Velin92 Velin92 deleted the mauroromito/enabling_push_notifications branch April 5, 2023 18:16
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.

Basic notification implementation
2 participants