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

[NEW] Menu for changing push notification settings #1396

Merged
merged 68 commits into from
Jul 30, 2018
Merged

[NEW] Menu for changing push notification settings #1396

merged 68 commits into from
Jul 30, 2018

Conversation

artur-ios-dev
Copy link
Contributor

@artur-ios-dev artur-ios-dev commented Mar 10, 2018 •
edited
Loading

@RocketChat/ios

Closes #288

  • add a controller for notifications preferences

  • switch cell

  • drop-down menu for a "choose cells"

  • implement the saveNotifications POST

  • localize everything

  • clean up the code

  • write tests

  • fix switch cell height when the description has more than 1 line

  • replace drop down menu with picker view (like system's add event)

  • notification's icon for subscription actions' list

  • [OPTIONAL] allow changing global notification preferences (accessible from Preferences menu?)

NOTES:

  • decide what to do with the getOneSubscription endpoint (I guess we have to implement it anyways to always get fresh settings from the server) // do we have to call it since Subscription is already being updated periodically in the app
  • update subscription object after saving settings so Subscription object that is assigned to Chat/Messages is always up to date [?] // NOTE: seems to be already updated periodically

notifications

houndci-bot
houndci-bot reviewed Mar 11, 2018
View reviewed changes
Rocket.Chat/NotificationsPreferencesViewModel.swift Outdated
internal let settings: [(title: String?, elements: [NotificationSettingModel])] = [
(title: nil, [
NotificationsSwitchCell.SettingModel(value: "1", type: .switch, leftTitle: "q", leftDescription: "w", rightTitle: "e", rightDescription: "r"),
NotificationsSwitchCell.SettingModel(value: "0", type: .switch, leftTitle: "t", leftDescription: "y", rightTitle: "u", rightDescription: "i"),
Copy link

Choose a reason for hiding this comment

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

Trailing Comma Violation: Collection literals should not have trailing commas. (trailing_comma)

Rocket.Chat/API/Clients/SettingsClient.swift Outdated
let api: AnyAPIFetcher

func fetchSettings() {
api.fetch(SettingsRequest(), succeeded: { result in
Copy link

Choose a reason for hiding this comment

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

Unused Closure Parameter Violation: Unused parameter "result" in a closure should be replaced with _. (unused_closure_parameter)

@rafaelks
Copy link
Contributor

@artrmz Thank you for the PR! Can you take a look on the conflicts please?

Artur Rymarz added 2 commits March 26, 2018 12:23
update notifications preferences view model
c18145d
Merge branch 'develop' into feature/288-notifications-preferences
daff2a8 
# Conflicts:
#	Rocket.Chat.xcodeproj/project.pbxproj
#	Rocket.Chat/Controllers/Settings/PreferencesViewController.swift
#	Rocket.Chat/Controllers/Settings/PreferencesViewModel.swift
#	Rocket.Chat/Resources/en.lproj/Localizable.strings
#	Rocket.Chat/Storyboards/Preferences.storyboard
@artur-ios-dev
Copy link
Contributor Author

Resolved conflicts, although there is still lots of work left, sadly I did not have much time lately :( Hopefully, I'll get back to it soon.

@rafaelks
Copy link
Contributor

@artrmz Thank you!

@artur-ios-dev
Merge branch 'develop' into feature/288-notifications-preferences
de4f7ac 
# Conflicts:
#	Rocket.Chat.xcodeproj/project.pbxproj
#	Rocket.Chat/Controllers/Settings/PreferencesViewController.swift
#	Rocket.Chat/Controllers/Settings/PreferencesViewModel.swift
#	Rocket.Chat/Storyboards/Preferences.storyboard
@artur-ios-dev
Merge branch 'feature/288-notifications-preferences' of https://githu…
dda0e6d 
…b.com/artrmz/Rocket.Chat.iOS into feature/288-notifications-preferences

# Conflicts:
#	Rocket.Chat/Storyboards/Preferences.storyboard
@Sameesunkaria
Copy link
Collaborator

Hey @artrmz !
I'm really looking forward to this feature. It would go well with #1504. 🎉

@artur-ios-dev
Copy link
Contributor Author

@Sameesunkaria Hey man! I am still quite short on time but will do my best to push it forward a little bit this/next week.

@Sameesunkaria
Copy link
Collaborator

@artrmz I'm not very familiar with the API stuff rn. But if you need any help, I have plenty of time. 😃

@artur-ios-dev
Copy link
Contributor Author

artur-ios-dev commented Apr 14, 2018 •
edited
Loading

@Sameesunkaria Thank you! Pretty much API (getting current notifications settings and saving new ones) is the last thing to do ;) Seems like RocketChat/Rocket.Chat#10128 has been merged so need to implement channels.notifications endpoint. I might implement it tonight if my kid lets me do that :D

@codecov
Copy link

codecov bot commented Apr 14, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@e5324c9). Click here to learn what that means.
The diff coverage is 7.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1396   +/-   ##
==========================================
  Coverage           ?   35.28%           
==========================================
  Files              ?      382           
  Lines              ?    17868           
  Branches           ?        0           
==========================================
  Hits               ?     6305           
  Misses             ?    11563           
  Partials           ?        0
Impacted Files Coverage Δ
...quests/Notifications/SaveNotificationRequest.swift 0% <0%> (ø)
Rocket.Chat/Extensions/StringExtensions.swift 81.42% <0%> (ø)
...rences/Notifications/NotificationsChooseCell.swift 0% <0%> (ø)
...tions/NotificationsPreferencesViewController.swift 0% <0%> (ø)
Rocket.Chat/Models/Subscription/Subscription.swift 22.58% <0%> (ø)
...rences/Notifications/NotificationsSwitchCell.swift 0% <0%> (ø)
Rocket.Chat/Views/PickerView/RCPickerView.swift 0% <0%> (ø)
...ifications/NotificationsPreferencesViewModel.swift 0% <0%> (ø)
Rocket.Chat/Extensions/EnumExtensions.swift 0% <0%> (ø)
...ontrollers/Chat/ChannelActionsViewController.swift 0% <0%> (ø)
... and 4 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 e5324c9...893aa77. Read the comment docs.

houndci-bot
houndci-bot reviewed Apr 14, 2018
View reviewed changes
Rocket.Chat/NotificationsPreferencesViewController.swift Outdated
self.viewModel.mobileAlertsModel.value.value = subscription.mobilePushNotifications ?? "placeholder"
self.viewModel.mailAlertsModel.value.value = subscription.emailNotifications ?? "placeholder"

}, errored: { error in
Copy link

Choose a reason for hiding this comment

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

Unused Closure Parameter Violation: Unused parameter "error" in a closure should be replaced with _. (unused_closure_parameter)

Rocket.Chat/NotificationsPreferencesViewController.swift Outdated
self.viewModel.counterModel.value.value = false // TODO: ???
// self.viewModel.desktopAlertsModel.value.value // TODO: ???
// self.viewModel.desktopAudioModel.value.value // TODO: ???
// self.viewModel.desktopSoundModel.value.value // TODO: ???
Copy link

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (???). (todo)

Rocket.Chat/NotificationsPreferencesViewController.swift Outdated
self.viewModel.enableModel.value.value = !subscription.disableNotifications
self.viewModel.counterModel.value.value = false // TODO: ???
// self.viewModel.desktopAlertsModel.value.value // TODO: ???
// self.viewModel.desktopAudioModel.value.value // TODO: ???
Copy link

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (???). (todo)

Rocket.Chat/NotificationsPreferencesViewController.swift Outdated

self.viewModel.enableModel.value.value = !subscription.disableNotifications
self.viewModel.counterModel.value.value = false // TODO: ???
// self.viewModel.desktopAlertsModel.value.value // TODO: ???
Copy link

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (???). (todo)

Rocket.Chat/NotificationsPreferencesViewController.swift Outdated
}

self.viewModel.enableModel.value.value = !subscription.disableNotifications
self.viewModel.counterModel.value.value = false // TODO: ???
Copy link

Choose a reason for hiding this comment

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

Todo Violation: TODOs should be resolved (???). (todo)

@artur-ios-dev
Copy link
Contributor Author

Tried to implement https://rocket.chat/docs/developer-guides/rest-api/subscriptions/getone/ but seems like values for notifications are missing or am I doing something wrong? @rafaelks

Getting such response (on open.rocket.chat):

Printing description of result.raw:
  "subscription" : {
    "name" : "ios-beta",
    "alert" : false,
    "_updatedAt" : "2018-04-14T22:30:40.275Z",
    "meta" : {
      "revision" : 48,
      "updated" : 1523745040279,
      "version" : 0,
      "created" : 1523628775399
    },
    "userMentions" : 0,
    "open" : true,
    "u" : {
      "name" : "Artrmz",
      "username" : "artrmz",
      "_id" : "GAcSEAjHQE43MDfud"
    },
    "groupMentions" : 0,
    "t" : "c",
    "ts" : "2018-04-06T16:40:54.884Z",
    "_id" : "evFXhSFJ8qnzizBo8",
    "rid" : "mD78xjvSctosNbSPr",
    "unread" : 0,
    "ls" : "2018-04-14T22:30:40.275Z",
    "customFields" : {

    },
    "fname" : "ios-beta"
  },
  "success" : true
}

@Sameesunkaria Sameesunkaria requested a review from filipealva July 24, 2018 05:14
@Sameesunkaria
Copy link
Collaborator

Sameesunkaria commented Jul 24, 2018 •
edited
Loading

Guys this PR is ready for review. @rafaelks @cardoso @filipealva

@artur-ios-dev
Copy link
Contributor Author

Thank you @Sameesunkaria for your contribution, looks much nicer now :)

@rafaelks
Copy link
Contributor

This is looking really really good, thanks a lot for the work on this!

Few things we can improve before we merge IMHO:

  1. The success message is danm big and message is cutting (see attachment);
  2. The "Save" button could be enabled only when there are changes; If there are no changes, the button should be disabled;
  3. Toggling the "Receive Notifications" is hiding all the cells with no animation at all, can we animate this?

img_5348

@Sameesunkaria
Copy link
Collaborator

@artrmz Can you please help me with the animations?

@artur-ios-dev
Copy link
Contributor Author

@Sameesunkaria Can take a look at that tomorrow morning 👍

@Sameesunkaria
Copy link
Collaborator

Sameesunkaria commented Jul 25, 2018 •
edited
Loading

  • The success message is danm big and message is cutting (see attachment);
  • The "Save" button could be enabled only when there are changes; If there are no changes, the button should be disabled;
  • Toggling the "Receive Notifications" is hiding all the cells with no animation at all, can we animate this?

kaiiiiiiiii
kaiiiiiiiii reviewed Jul 25, 2018
View reviewed changes
Rocket.Chat/Resources/cs.lproj/Localizable.strings Outdated
@@ -366,7 +366,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes I hate that Xcode doesn't have autocorrect for strings. 🙈

Rocket.Chat/Resources/de.lproj/Localizable.strings Outdated
@@ -366,7 +366,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/el.lproj/Localizable.strings Outdated
@@ -366,7 +366,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Copy link
Collaborator

Choose a reason for hiding this comment

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

still missing this one @Sameesunkaria

Rocket.Chat/Resources/en.lproj/Localizable.strings Outdated
@@ -365,7 +365,7 @@
"myaccount.settings.notifications.duration.default" = "Default";
"myaccount.settings.notifications.duration.seconds" = "seconds";

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences.";
"alert.update_notifications_preferences_success.title" = "Perferences saved";
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/es.lproj/Localizable.strings Outdated
@@ -365,7 +365,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/fr.lproj/Localizable.strings Outdated
@@ -364,7 +364,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/pl.lproj/Localizable.strings Outdated
@@ -365,7 +365,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/pt-BR.lproj/Localizable.strings Outdated
@@ -366,7 +366,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/pt-PT.lproj/Localizable.strings Outdated
@@ -363,7 +363,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

Rocket.Chat/Resources/ru.lproj/Localizable.strings Outdated
@@ -365,7 +365,7 @@
"myaccount.settings.notifications.duration.default" = "Default"; // TODO
"myaccount.settings.notifications.duration.seconds" = "seconds"; // TODO

"alert.update_notifications_preferences_success.title" = "Successfully updated your notification preferences."; // TODO
"alert.update_notifications_preferences_success.title" = "Perferences saved"; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo :) "Preferences saved".

@artur-ios-dev
Copy link
Contributor Author

@Sameesunkaria I am done with enabling/disabling save button although not sure yet if I want to push that :) We either have to request subscription to make sure we are synced with the server or leave the controller after saving, not sure what is better... additional request or closing controller 🤔

Will let know once I finish the animation.

@rafaelks
Copy link
Contributor

@artrmz That makes sense... if it's too complicated let's leave the button the way it is now.

@artur-ios-dev
Copy link
Contributor Author

@rafaelks @Sameesunkaria Pushed the change to enable/disable "Save button" as needed.

@rafaelks
Copy link
Contributor

@artrmz @Sameesunkaria Looking great to me, gonna review the code!

@rafaelks rafaelks changed the title [FEATURE] Menu for changing push notification settings [NEW] Menu for changing push notification settings Jul 30, 2018
rafaelks
rafaelks approved these changes Jul 30, 2018
View reviewed changes
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.

Looking really nice! 👍

@rafaelks rafaelks merged commit 101d5ed into RocketChat:develop Jul 30, 2018
@Sameesunkaria
Copy link
Collaborator

Thanks @artrmz for all your hard work on this one! 🎉

@rafaelks
Copy link
Contributor

Yes, thank you very much for that @artrmz, many people are going to be happy with this PR!!! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kaiiiiiiiii kaiiiiiiiii kaiiiiiiiii left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@Sameesunkaria Sameesunkaria Sameesunkaria left review comments

@rafaelks rafaelks rafaelks approved these changes

@cardoso cardoso Awaiting requested review from cardoso

@filipealva filipealva Awaiting requested review from filipealva

Assignees

@rafaelks rafaelks

@Sameesunkaria Sameesunkaria

Labels
🎉 feature 👀 needs review
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.

[NEW] Menu for changing push notification settings
7 participants
@artur-ios-dev @rafaelks @Sameesunkaria @thiagosanchz @kaiiiiiiiii @cardoso @houndci-bot