Skip to content
This repository has been archived by the owner on Jun 7, 2020. It is now read-only.

[NEW] In-app notifications #1504

Merged
merged 51 commits into from Apr 13, 2018

Conversation

Sameesunkaria
Copy link
Collaborator

@Sameesunkaria Sameesunkaria commented Apr 8, 2018

@RocketChat/ios

This PR adds the functionality of displaying in-app notifications. The notifications are delivered based on the stream-notify-user/notification steam.

Component checklist

  • Display notifications
  • Animate the transition on screen
  • Responding to taps
  • Sounds and vibrations
  • Ability to swipe the notification off the screen
  • Tests

Issues to address separately (Server-Side)

Closes #1125
Closes #1339

@codecov
Copy link

codecov bot commented Apr 8, 2018

Codecov Report

Merging #1504 into develop will increase coverage by 2.39%.
The diff coverage is 40.93%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1504      +/-   ##
===========================================
+ Coverage    34.15%   36.54%   +2.39%     
===========================================
  Files          268      277       +9     
  Lines        12726    12896     +170     
===========================================
+ Hits          4346     4713     +367     
+ Misses        8380     8183     -197
Impacted Files Coverage Δ
Rocket.Chat/Models/Subscription/Subscription.swift 100% <ø> (ø) ⬆️
...iews/Notification/TransparentToTouchesWindow.swift 0% <0%> (ø)
Rocket.Chat/Managers/PushManager.swift 59.32% <0%> (-1.55%) ⬇️
...odel/SubscriptionManager/SubscriptionManager.swift 0% <0%> (ø)
Rocket.Chat/Managers/Socket/SocketManager.swift 68.34% <0%> (+6.75%) ⬆️
...ubscriptionManager/SubscriptionManagerSearch.swift 0% <0%> (ø)
...ubscriptionManager/SubscriptionManagerTyping.swift 0% <0%> (ø)
Rocket.Chat/Models/User/UserUtils.swift 100% <100%> (ø) ⬆️
...ket.Chat/Views/Notification/NotificationView.swift 100% <100%> (ø)
Rocket.Chat/AppDelegate.swift 54.54% <100%> (+8.59%) ⬆️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e355a75...0d7e844. Read the comment docs.

@Sameesunkaria
Copy link
Collaborator Author

@rafaelks @cardoso The in-app notifications should be feature complete now. I'll be committing the tests tomorrow. In the meanwhile, it would be awesome if you guys could try it out and give me some feedback on the implementation of the feature.

func testDirectMessageNotificationDecoding() {
let notificationJson =
"""
{

Choose a reason for hiding this comment

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

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

func testChannelNotificationDecoding() {
let notificationJson =
"""
{

Choose a reason for hiding this comment

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

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

body: "Hey!",
rid: "UUUUUUUU"
)

Choose a reason for hiding this comment

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

Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

@cardoso cardoso requested a review from rafaelks April 11, 2018 23:56
Copy link
Contributor

@rafaelks rafaelks left a comment

Choose a reason for hiding this comment

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

First of all, the result of this PR is looking very good @Sameesunkaria! But, I was able to receive notifications with the app opened. Can we disable the notifications when the app is opened and the WebSocket is connected?

img_0052

guard let notificationView = notificationView else { return }

notificationView.displayNotification(title: title, body: body, username: username)
playSound()
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about not playing any sound for now? Until we have a setting for that, I would with no sound. Let's just comment the code maybe :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I was kinda on the fence about it anyway.

self.view.layoutIfNeeded()
}

timer = Timer.scheduledTimer(withTimeInterval: notificationVisibleDuration, repeats: false) { _ in
Copy link
Contributor

Choose a reason for hiding this comment

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

[weak self]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😅

@Sameesunkaria
Copy link
Collaborator Author

@rafaelks You'll have to test the outcome with your dev profile. I am not able to get my APNS cert to work with RC.

@rafaelks
Copy link
Contributor

Looking amazing @Sameesunkaria!!! Thank you! 👏

@rafaelks rafaelks merged commit bf3f3d4 into RocketChat:develop Apr 13, 2018
@Sameesunkaria Sameesunkaria deleted the local-notifications branch April 13, 2018 19:49
@rafaelks rafaelks added this to the 2.2.0 milestone Apr 13, 2018
@rafaelks rafaelks added this to Desirable in Cycle 04: 04/05 ~ 04/27 via automation Apr 13, 2018
@rafaelks rafaelks moved this from Desirable to Done in Cycle 04: 04/05 ~ 04/27 Apr 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants