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

2350: Show snackbar instead of opening settings #2960

Merged
merged 4 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions native/src/Navigator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,17 @@ const Stack = createStackNavigator<RoutesParamsType>()

const Navigator = (): ReactElement | null => {
const showSnackbar = useSnackbar()
const { settings, cityCode, changeCityCode, languageCode, updateSettings } = useAppContext()
const appContext = useAppContext()
const { settings, cityCode, changeCityCode, updateSettings } = appContext
const navigation = useNavigation<NavigationProps<RoutesType>>()
const [initialRoute, setInitialRoute] = useState<InitialRouteType>(null)

// Preload cities
const { data: cities, error: citiesError, refresh: refreshCities } = useLoadCities()

useEffect(() => {
initialPushNotificationRequest(cityCode, languageCode).catch(reportError)
}, [cityCode, languageCode])
initialPushNotificationRequest(appContext).catch(reportError)
}, [appContext])

useForegroundPushNotificationListener({ showSnackbar, navigate: navigation.navigate })

Expand Down
8 changes: 4 additions & 4 deletions native/src/components/ExportEventButton.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { DateTime } from 'luxon'
import React, { ReactElement, useState } from 'react'
import { useTranslation } from 'react-i18next'
import { Linking, Platform } from 'react-native'
import { Platform } from 'react-native'
import RNCalendarEvents, { Calendar, CalendarEventWritable, RecurrenceFrequency } from 'react-native-calendar-events'
import { PERMISSIONS, requestMultiple } from 'react-native-permissions'
import { PERMISSIONS, openSettings, requestMultiple } from 'react-native-permissions'
import { Frequency } from 'rrule'
import styled from 'styled-components/native'

Expand Down Expand Up @@ -89,8 +89,8 @@ const ExportEventButton = ({ event }: ExportEventButtonType): ReactElement => {
showSnackbar({
text: 'noCalendarPermission',
positiveAction: {
label: t('settings'),
onPress: Linking.openSettings,
label: t('layout:settings'),
onPress: openSettings,
steffenkleinle marked this conversation as resolved.
Show resolved Hide resolved
},
})
return
Expand Down
9 changes: 5 additions & 4 deletions native/src/contexts/AppContextProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,44 +34,45 @@

const AppContextProvider = ({ children }: AppContextProviderProps): ReactElement | null => {
const [settings, setSettings] = useState<SettingsType | null>(null)
const allowPushNotifications = !!settings?.allowPushNotifications
const cityCode = settings?.selectedCity
const languageCode = settings?.contentLanguage
const { i18n } = useTranslation()
const uiLanguage = i18n.languages[0]

useEffect(() => {
appSettings.loadSettings().then(setSettings).catch(reportError)
}, [])

const updateSettings = useCallback((settings: Partial<SettingsType>) => {
setSettings(oldSettings => (oldSettings ? { ...oldSettings, ...settings } : null))
appSettings.setSettings(settings).catch(reportError)
}, [])

const changeCityCode = useCallback(
(newCityCode: string | null): void => {
updateSettings({ selectedCity: newCityCode })
if (languageCode && cityCode) {
unsubscribeNews(cityCode, languageCode).catch(reportError)
}
if (languageCode && newCityCode) {
subscribeNews(newCityCode, languageCode).catch(reportError)
subscribeNews({ cityCode: newCityCode, languageCode, allowPushNotifications }).catch(reportError)
}
},
[updateSettings, cityCode, languageCode],
[updateSettings, cityCode, languageCode, allowPushNotifications],
)

const changeLanguageCode = useCallback(
(newLanguageCode: string): void => {
updateSettings({ contentLanguage: newLanguageCode })
if (cityCode && languageCode) {
unsubscribeNews(cityCode, languageCode).catch(reportError)
}
if (cityCode) {
subscribeNews(cityCode, newLanguageCode).catch(reportError)
subscribeNews({ cityCode, languageCode: newLanguageCode, allowPushNotifications }).catch(reportError)
}
},
[updateSettings, cityCode, languageCode],
[updateSettings, cityCode, languageCode, allowPushNotifications],

Check notice on line 75 in native/src/contexts/AppContextProvider.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

ℹ Getting worse: Complex Method

AppContextProvider increases in cyclomatic complexity from 27 to 28, threshold = 10. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
)

useEffect(() => {
Expand Down
20 changes: 16 additions & 4 deletions native/src/contexts/__tests__/AppContextProvider.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import AsyncStorage from '@react-native-async-storage/async-storage'

Check notice on line 1 in native/src/contexts/__tests__/AppContextProvider.spec.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

✅ Getting better: Code Duplication

reduced similar code in: 'should deselect city'. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
import { fireEvent, waitFor } from '@testing-library/react-native'
import { mocked } from 'jest-mock'
import React, { useContext } from 'react'
Expand Down Expand Up @@ -135,7 +135,7 @@
expect(await appSettings.loadSettings()).toMatchObject({ selectedCity: 'hallo' })
expect(setSettings).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledWith('hallo', 'de')
expect(subscribeNews).toHaveBeenCalledWith({ cityCode: 'hallo', languageCode: 'de', allowPushNotifications: true })
})

it('should select city', async () => {
Expand All @@ -150,7 +150,11 @@
expect(await appSettings.loadSettings()).toMatchObject({ selectedCity: 'augsburg' })
expect(setSettings).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledWith('augsburg', 'de')
expect(subscribeNews).toHaveBeenCalledWith({
cityCode: 'augsburg',
languageCode: 'de',
allowPushNotifications: true,
})
expect(unsubscribeNews).not.toHaveBeenCalled()
})

Expand All @@ -166,7 +170,11 @@
expect(await appSettings.loadSettings()).toMatchObject({ selectedCity: 'augsburg' })
expect(setSettings).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledWith('augsburg', 'de')
expect(subscribeNews).toHaveBeenCalledWith({
cityCode: 'augsburg',
languageCode: 'de',
allowPushNotifications: true,
})
expect(unsubscribeNews).toHaveBeenCalledTimes(1)
expect(unsubscribeNews).toHaveBeenCalledWith('muenchen', 'de')
})
Expand Down Expand Up @@ -198,7 +206,11 @@
expect(await appSettings.loadSettings()).toMatchObject({ contentLanguage: 'ar' })
expect(setSettings).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledTimes(1)
expect(subscribeNews).toHaveBeenCalledWith('muenchen', 'ar')
expect(subscribeNews).toHaveBeenCalledWith({
cityCode: 'muenchen',
languageCode: 'ar',
allowPushNotifications: true,
})
expect(unsubscribeNews).toHaveBeenCalledTimes(1)
expect(unsubscribeNews).toHaveBeenCalledWith('muenchen', 'de')
})
Expand Down
4 changes: 2 additions & 2 deletions native/src/hooks/useCityAppContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import { useContext } from 'react'

import { AppContext, AppContextType } from '../contexts/AppContextProvider'

type UseCityAppContextReturn = AppContextType & {
export type CityAppContext = AppContextType & {
cityCode: string
}

export const useAppContext = (): AppContextType => useContext(AppContext)

const useCityAppContext = (): UseCityAppContextReturn => {
const useCityAppContext = (): CityAppContext => {
const { cityCode, ...context } = useAppContext()
if (!cityCode) {
throw new Error('City code not set!')
Expand Down
42 changes: 13 additions & 29 deletions native/src/routes/Settings.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { ReactElement, useContext } from 'react'
import React, { ReactElement } from 'react'
import { useTranslation } from 'react-i18next'
import { SectionList, SectionListData } from 'react-native'
import styled from 'styled-components/native'
Expand All @@ -10,10 +10,8 @@ import Layout from '../components/Layout'
import SettingItem from '../components/SettingItem'
import ItemSeparator from '../components/base/ItemSeparator'
import { NavigationProps } from '../constants/NavigationTypes'
import { AppContext } from '../contexts/AppContextProvider'
import { useAppContext } from '../hooks/useCityAppContext'
import useCityAppContext from '../hooks/useCityAppContext'
import useSnackbar from '../hooks/useSnackbar'
import { SettingsType } from '../utils/AppSettings'
import createSettingsSections, { SettingsSectionType } from '../utils/createSettingsSections'
import { log, reportError } from '../utils/sentry'

Expand All @@ -31,36 +29,25 @@ const SectionHeader = styled.Text`
`

const Settings = ({ navigation }: SettingsProps): ReactElement => {
const { settings, updateSettings } = useAppContext()
const { cityCode, languageCode } = useContext(AppContext)
const appContext = useCityAppContext()
const showSnackbar = useSnackbar()
const { t } = useTranslation('settings')
const { settings } = appContext

const setSetting = async (
changeSetting: (settings: SettingsType) => Partial<SettingsType>,
changeAction?: (settings: SettingsType) => Promise<boolean>,
) => {
const safeOnPress = (update: () => Promise<void> | void) => () => {
const oldSettings = settings
const newSettings = { ...oldSettings, ...changeSetting(settings) }
updateSettings(newSettings)

try {
const successful = changeAction ? await changeAction(newSettings) : true

if (!successful) {
updateSettings(oldSettings)
}
} catch (e) {
update()?.catch(e => {
log('Failed to persist settings.', 'error')
steffenkleinle marked this conversation as resolved.
Show resolved Hide resolved
reportError(e)
updateSettings(oldSettings)
}
appContext.updateSettings(oldSettings)
showSnackbar({ text: t('error:unknownError') })
})
steffenkleinle marked this conversation as resolved.
Show resolved Hide resolved
}

const renderItem = ({ item }: { item: SettingsSectionType }) => {
const { getSettingValue, ...otherProps } = item
const { getSettingValue, onPress, ...otherProps } = item
const value = !!(getSettingValue && getSettingValue(settings))
return <SettingItem value={value} key={otherProps.title} {...otherProps} />
return <SettingItem value={value} key={otherProps.title} onPress={safeOnPress(onPress)} {...otherProps} />
}

const renderSectionHeader = ({ section: { title } }: { section: SectionType }) => {
Expand All @@ -72,13 +59,10 @@ const Settings = ({ navigation }: SettingsProps): ReactElement => {
}

const sections = createSettingsSections({
setSetting,
t,
languageCode,
cityCode,
appContext,
navigation,
settings,
showSnackbar,
t,
})

return (
Expand Down
33 changes: 18 additions & 15 deletions native/src/testing/TestingAppContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,30 @@ import React, { ReactElement, ReactNode } from 'react'
import { AppContext, AppContextType } from '../contexts/AppContextProvider'
import { defaultSettings, SettingsType } from '../utils/AppSettings'

const TestingAppContext = ({
children,
type TestingAppContextParams = { settings?: Partial<SettingsType> } & Omit<Partial<AppContextType>, 'settings'>

export const testingAppContext = ({
settings = {},
cityCode = 'augsburg',
languageCode = 'de',
changeCityCode = jest.fn(),
changeLanguageCode = jest.fn(),
updateSettings = jest.fn(),
}: {
settings?: Partial<SettingsType>
children: ReactNode
} & Omit<Partial<AppContextType>, 'settings'>): ReactElement => {
const context = {
settings: { ...defaultSettings, ...settings },
cityCode,
languageCode,
updateSettings,
changeCityCode,
changeLanguageCode,
}
}: TestingAppContextParams): AppContextType => ({
settings: { ...defaultSettings, ...settings },
cityCode,
languageCode,
updateSettings,
changeCityCode,
changeLanguageCode,
})

const TestingAppContextProvider = ({
children,
...props
}: { children: ReactNode } & TestingAppContextParams): ReactElement => {
const context = testingAppContext(props)
return <AppContext.Provider value={context}>{children}</AppContext.Provider>
}

export default TestingAppContext
export default TestingAppContextProvider
37 changes: 26 additions & 11 deletions native/src/utils/PushNotificationsManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ import { LOCAL_NEWS_TYPE, NEWS_ROUTE, NonNullableRouteInformationType } from 'sh
import { SnackbarType } from '../components/SnackbarContainer'
import { RoutesType } from '../constants/NavigationTypes'
import buildConfig from '../constants/buildConfig'
import { AppContextType } from '../contexts/AppContextProvider'
import urlFromRouteInformation from '../navigation/url'
import appSettings from './AppSettings'
import { log, reportError } from './sentry'

type UpdateSettingsType = (settings: { allowPushNotifications: boolean }) => void

type Message = FirebaseMessagingTypes.RemoteMessage & {
notification: { title: string }
data: {
Expand All @@ -30,7 +32,7 @@ const importFirebaseMessaging = async (): Promise<() => FirebaseMessagingTypes.M
export const pushNotificationsEnabled = (): boolean =>
buildConfig().featureFlags.pushNotifications && !buildConfig().featureFlags.floss

export const requestPushNotificationPermission = async (): Promise<boolean> => {
export const requestPushNotificationPermission = async (updateSettings: UpdateSettingsType): Promise<boolean> => {
if (!pushNotificationsEnabled()) {
log('Push notifications disabled, no permissions requested.')
return false
Expand All @@ -41,7 +43,7 @@ export const requestPushNotificationPermission = async (): Promise<boolean> => {

if (permissionStatus !== RESULTS.GRANTED) {
log(`Permission denied, disabling push notifications in settings.`)
await appSettings.setSettings({ allowPushNotifications: false })
updateSettings({ allowPushNotifications: false })
Copy link
Member Author

Choose a reason for hiding this comment

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

This led to unsynchronized settings.

}

return permissionStatus === RESULTS.GRANTED
Expand All @@ -65,15 +67,27 @@ export const unsubscribeNews = async (city: string, language: string): Promise<v
}
log(`Unsubscribed from ${topic} topic!`)
}
export const subscribeNews = async (city: string, language: string, skipSettingsCheck = false): Promise<void> => {

type SubscribeNewsParams = {
cityCode: string
languageCode: string
allowPushNotifications: boolean
skipSettingsCheck?: boolean
}

export const subscribeNews = async ({
cityCode,
languageCode,
allowPushNotifications,
skipSettingsCheck = false,
}: SubscribeNewsParams): Promise<void> => {
try {
const { allowPushNotifications } = await appSettings.loadSettings()
if (!pushNotificationsEnabled() || (!allowPushNotifications && !skipSettingsCheck)) {
log('Push notifications disabled, subscription skipped.')
return
}

const topic = newsTopic(city, language)
const topic = newsTopic(cityCode, languageCode)

const messaging = await importFirebaseMessaging()
await messaging().subscribeToTopic(topic)
Expand Down Expand Up @@ -154,7 +168,6 @@ export const backgroundAppStatePushNotificationListener = (listener: (url: strin
importFirebaseMessaging()
.then(messaging => {
const onReceiveURL = ({ url }: { url: string }) => listener(url)

const onReceiveURLListener = Linking.addListener('url', onReceiveURL)

const unsubscribeNotification = messaging().onNotificationOpenedApp(message =>
Expand All @@ -175,13 +188,15 @@ export const backgroundAppStatePushNotificationListener = (listener: (url: strin
// Since Android 13 and iOS 17 an explicit permission request is needed, otherwise push notifications are not received.
// Therefore request the permissions once if not yet granted and subscribe to the current channel if successful.
// See https://github.com/digitalfabrik/integreat-app/issues/2438 and https://github.com/digitalfabrik/integreat-app/issues/2655
export const initialPushNotificationRequest = async (cityCode: string | null, languageCode: string): Promise<void> => {
const { allowPushNotifications } = await appSettings.loadSettings()
export const initialPushNotificationRequest = async (appContext: AppContextType): Promise<void> => {
const { cityCode, languageCode, settings, updateSettings } = appContext
const { allowPushNotifications } = settings

const pushNotificationPermissionGranted = (await checkNotifications()).status === RESULTS.GRANTED
if (!pushNotificationPermissionGranted && allowPushNotifications) {
const success = await requestPushNotificationPermission()
const success = await requestPushNotificationPermission(updateSettings)
if (success && cityCode) {
await subscribeNews(cityCode, languageCode)
await subscribeNews({ cityCode, languageCode, allowPushNotifications })
}
}
}
Loading
Loading