Skip to content

Conversation

@baronhsieh2005
Copy link
Contributor

Our app can receive notifications now!

Changes

  • Using FCM, our app could receive notifications by sending their FCM Registration token, a device-based token that can be used by admin to send notifications by FCM service, to the backend.
  • For every new token generated, we would store/update it in SharedPreferences. Therefore, to fetch notification tokens, we can easily do so reading SharedPreferences.
  • Currently, we implemented network requests by posting the registration token to the backend so they can send notifications as admin and deleting the registration token from the backend so users won't receive notifications when they are logged out.
  • There are more to come. For now, this implementation would be sufficient to send pca notifications by the add/drop period. We would add new notification preferences (so users can customize what notifications they want to receive) as new notifications are being introduced. @Divak2004 is working on the settings UI.

Documentation and Testing:

ActivityResultContracts.RequestPermission(),
) { isGranted: Boolean ->
if (isGranted) {
// FCM SDK (and your app) can post notifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

If isGranted is true, does that just mean you can send notifications even though nothing is written here.

Copy link
Contributor

Choose a reason for hiding this comment

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

doubling this question

Is this something where we can choose to do something (ex: sending a "success!" message to the user), but it's also okay not to do anything (since notifications will be shown as enabled in the background?)

Copy link
Member

@meiron03 meiron03 Nov 22, 2024

Choose a reason for hiding this comment

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

I don't think we have to do anything, but having TODOs in main is generally considered bad practice. What behavior should we expect if notifications aren't enabled? Is this information generally available or should it be stored somewhere? (oops, answered by scrolling down LOL)

I don't think we have to do anything then. How do other ppl generally use this function?

) {
// FCM SDK (and your app) can post notifications.
} else if (shouldShowRequestPermissionRationale(Manifest.permission.POST_NOTIFICATIONS)) {
// TODO: display an educational UI explaining to the user the features that will be enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some method to keep track of the user already rejecting notifications?

Copy link
Contributor

@Akula112233 Akula112233 Nov 19, 2024

Choose a reason for hiding this comment

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

It seems the "shouldShowRequestPermissionRationale" function actually checks the Manifest.permission.POST_NOTIFICATIONS to see whether these permissions was just rejected (meaning ask again on-create), or if it was rejected + "don't show again" box was checked (meaning don't ask again on-create)

When a user rejects, the specific type of rejection is stored within Manifest.permission.POST_NOTIFICATIONS. Pretty Cool

Copy link
Member

Choose a reason for hiding this comment

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

again, try to clean up TODOs. Personally, I don't believe we really need an explanation for this and am comfortable with doing nothing.

Copy link
Contributor

@Divak2004 Divak2004 left a comment

Choose a reason for hiding this comment

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

Left some questions above

Copy link
Member

@meiron03 meiron03 left a comment

Choose a reason for hiding this comment

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

See comments. Also, figure out how u want to update token in backend when onNewToken() is called.

testImplementation platform(libs.androidx.compose.bom)
androidTestImplementation platform(libs.androidx.compose.bom)
implementation platform(libs.firebase.bom)
implementation(libs.firebase.messaging)
Copy link
Member

Choose a reason for hiding this comment

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

why does this have parens but the other imports don't? Can we standardize formatting?

setTheme(R.style.DarkBackground)
}
Utils.getCurrentSystemTime()
askNotificationPermission()
Copy link
Member

Choose a reason for hiding this comment

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

why are we asking for notification permission every time the activity starts? It is probably annoying if they don't want notifications and the app keeps asking... I feel like the app should just ask once.

Copy link
Member

Choose a reason for hiding this comment

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

udp: I didn't realize that android had a "never ask again" feature. Someone correct me if I'm wrong, but this case should be handled right.

) {
// FCM SDK (and your app) can post notifications.
} else if (shouldShowRequestPermissionRationale(Manifest.permission.POST_NOTIFICATIONS)) {
// TODO: display an educational UI explaining to the user the features that will be enabled
Copy link
Member

Choose a reason for hiding this comment

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

again, try to clean up TODOs. Personally, I don't believe we really need an explanation for this and am comfortable with doing nothing.

<color name="light_blue_200">#FF81D4FA</color>
<color name="light_blue_600">#FF039BE5</color>
<color name="light_blue_900">#FF01579B</color>
<color name="penn_red">#990000</color>
Copy link
Member

Choose a reason for hiding this comment

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

we already have PennRed... Please remove one of them.

class LoginWebviewViewmodel : ViewModel() {
suspend fun sendToken(
mNotificationAPI: NotificationAPI,
notGuest: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

can we negate this here. (I know our guest mode stuff is kinda fucked up but notGuest is just an extremely cursed variable name)

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.

5 participants