-
Notifications
You must be signed in to change notification settings - Fork 19
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
removing enable api testing #338
base: develop
Are you sure you want to change the base?
Changes from 5 commits
080368b
517924d
db719d5
7d16a5c
3a01657
c35a46e
2e26039
253b652
4922ba1
b697bd1
0bf9c64
f2edf61
6683770
55658b5
d6c61b7
e17cac1
8298d99
5f93fa1
f9fa87d
cdd46ca
39c2ec9
91607ea
f4c45c3
9b8908e
6a70641
dce8cc6
efe2ff9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import { | |
import { | ||
urlBase64ToUint8Array | ||
} from '../util/encoder' | ||
import { enablePush } from './webPushPrompt/prompt' | ||
import { setNotificationHandlerValues, processSoftPrompt } from './webPushPrompt/prompt' | ||
|
||
export default class NotificationHandler extends Array { | ||
#oldValues | ||
|
@@ -31,14 +31,23 @@ export default class NotificationHandler extends Array { | |
this.#account = account | ||
} | ||
|
||
push (...displayArgs) { | ||
setupWebPush (displayArgs) { | ||
this.#setUpWebPush(displayArgs) | ||
return 0 | ||
} | ||
|
||
enable (options = {}) { | ||
const { swPath, skipDialog } = options | ||
enablePush(this.#logger, this.#account, this.#request, swPath, skipDialog) | ||
push (...displayArgs) { | ||
const isWebPushConfigPresent = StorageManager.readFromLSorCookie('webPushConfigResponseReceived') | ||
setNotificationHandlerValues({ | ||
logger: this.#logger, | ||
account: this.#account, | ||
request: this.#request, | ||
displayArgs | ||
}) | ||
if (isWebPushConfigPresent) { | ||
processSoftPrompt() | ||
} else { | ||
StorageManager.saveToLSorCookie('notificationPushCalled', true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this variable is used to defer the push call, consider renaming it to something like |
||
} | ||
} | ||
|
||
_processOldValues () { | ||
|
@@ -138,6 +147,9 @@ export default class NotificationHandler extends Array { | |
subscribeObj.applicationServerKey = urlBase64ToUint8Array(this.#fcmPublicKey) | ||
} | ||
|
||
const softPromptCard = document.getElementById('pnWrapper') | ||
const oldSoftPromptCard = document.getElementById('wzrk_wrapper') | ||
|
||
serviceWorkerRegistration.pushManager.subscribe(subscribeObj) | ||
.then((subscription) => { | ||
this.#logger.info('Service Worker registered. Endpoint: ' + subscription.endpoint) | ||
|
@@ -160,11 +172,19 @@ export default class NotificationHandler extends Array { | |
subscriptionCallback() | ||
} | ||
const existingBellWrapper = document.getElementById('bell_wrapper') | ||
|
||
if (existingBellWrapper) { | ||
existingBellWrapper.parentNode.removeChild(existingBellWrapper) | ||
} | ||
if (softPromptCard) { | ||
softPromptCard.parentNode.removeChild(softPromptCard) | ||
} | ||
if (oldSoftPromptCard) { | ||
oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) | ||
} | ||
}).catch((error) => { | ||
// unsubscribe from webpush if error | ||
console.log('rejected subscription') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use logger |
||
serviceWorkerRegistration.pushManager.getSubscription().then((subscription) => { | ||
if (subscription !== null) { | ||
subscription.unsubscribe().then((successful) => { | ||
|
@@ -180,6 +200,12 @@ export default class NotificationHandler extends Array { | |
} | ||
}) | ||
this.#logger.error('Error subscribing: ' + error) | ||
if (softPromptCard) { | ||
softPromptCard.parentNode.removeChild(softPromptCard) | ||
} | ||
if (oldSoftPromptCard) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add version Bump and Changelog. |
||
oldSoftPromptCard.parentNode.removeChild(oldSoftPromptCard) | ||
} | ||
}) | ||
}).catch((err) => { | ||
this.#logger.error('error registering service worker: ' + err) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,12 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||
import { getBellIconStyles, getBoxPromptStyles } from './promptStyles.js' | ||||||||||||||||||||||||||||||||||||||
import { WEBPUSH_CONFIG } from '../../util/constants.js' | ||||||||||||||||||||||||||||||||||||||
import { isObject } from '../../util/datatypes.js' | ||||||||||||||||||||||||||||||||||||||
import { StorageManager, $ct } from '../../util/storage.js' | ||||||||||||||||||||||||||||||||||||||
import NotificationHandler from '../notification.js' | ||||||||||||||||||||||||||||||||||||||
import { BELL_BASE64, PROMPT_BELL_BASE64 } from './promptConstants.js' | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let appServerKey = null | ||||||||||||||||||||||||||||||||||||||
let swPath = '/clevertap_sw.js' | ||||||||||||||||||||||||||||||||||||||
let notificationHandler = null | ||||||||||||||||||||||||||||||||||||||
let logger = null | ||||||||||||||||||||||||||||||||||||||
let account = null | ||||||||||||||||||||||||||||||||||||||
let request = null | ||||||||||||||||||||||||||||||||||||||
let displayArgs = null | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export const setNotificationHandlerValues = (notificationValues = {}) => { | ||||||||||||||||||||||||||||||||||||||
logger = notificationValues.logger | ||||||||||||||||||||||||||||||||||||||
account = notificationValues.account | ||||||||||||||||||||||||||||||||||||||
request = notificationValues.request | ||||||||||||||||||||||||||||||||||||||
displayArgs = notificationValues.displayArgs | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export const processWebPushConfig = (webPushConfig, logger, request) => { | ||||||||||||||||||||||||||||||||||||||
const _pushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {} | ||||||||||||||||||||||||||||||||||||||
|
@@ -21,7 +33,41 @@ export const processWebPushConfig = (webPushConfig, logger, request) => { | |||||||||||||||||||||||||||||||||||||
enablePush(logger, null, request) | ||||||||||||||||||||||||||||||||||||||
} else if (JSON.stringify(_pushConfig) !== JSON.stringify(webPushConfig)) { | ||||||||||||||||||||||||||||||||||||||
updatePushConfig() | ||||||||||||||||||||||||||||||||||||||
StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true) | ||||||||||||||||||||||||||||||||||||||
const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled') | ||||||||||||||||||||||||||||||||||||||
if (isNotificationPushCalled) { | ||||||||||||||||||||||||||||||||||||||
processSoftPrompt() | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling for storage operations Storage operations could fail (e.g., in private browsing mode). Consider adding try-catch blocks and fallback behavior. - StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true)
- const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
- if (isNotificationPushCalled) {
- processSoftPrompt()
- }
+ try {
+ StorageManager.saveToLSorCookie('webPushConfigResponseReceived', true)
+ const isNotificationPushCalled = StorageManager.readFromLSorCookie('notificationPushCalled')
+ if (isNotificationPushCalled) {
+ processSoftPrompt()
+ }
+ } catch (error) {
+ logger?.error('Failed to process web push config:', error)
+ // Fallback: Attempt to process soft prompt anyway
+ processSoftPrompt()
+ } 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export const processSoftPrompt = () => { | ||||||||||||||||||||||||||||||||||||||
const webPushConfig = StorageManager.readFromLSorCookie(WEBPUSH_CONFIG) || {} | ||||||||||||||||||||||||||||||||||||||
const notificationHandler = new NotificationHandler({ logger, session: {}, request, account }) | ||||||||||||||||||||||||||||||||||||||
if (!(Object.keys(webPushConfig).length > 0)) { | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have a null check on
|
||||||||||||||||||||||||||||||||||||||
notificationHandler.setApplicationServerKey(appServerKey) | ||||||||||||||||||||||||||||||||||||||
notificationHandler.setupWebPush(displayArgs) | ||||||||||||||||||||||||||||||||||||||
return 0 | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: return |
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
const { showBox, showBellIcon, boxType } = webPushConfig | ||||||||||||||||||||||||||||||||||||||
const { serviceWorkerPath, skipDialog } = parseDisplayArgs(displayArgs) | ||||||||||||||||||||||||||||||||||||||
if (showBellIcon || (showBox && boxType === 'new')) { | ||||||||||||||||||||||||||||||||||||||
enablePush(logger, account, request, serviceWorkerPath, skipDialog) | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
if (showBox && boxType === 'old') { | ||||||||||||||||||||||||||||||||||||||
notificationHandler.setApplicationServerKey(appServerKey) | ||||||||||||||||||||||||||||||||||||||
notificationHandler.setupWebPush(displayArgs) | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export const parseDisplayArgs = (displayArgs) => { | ||||||||||||||||||||||||||||||||||||||
if (displayArgs.length === 1 && isObject(displayArgs[0])) { | ||||||||||||||||||||||||||||||||||||||
const { serviceWorkerPath, skipDialog } = displayArgs[0] | ||||||||||||||||||||||||||||||||||||||
return { serviceWorkerPath, skipDialog } | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
return { serviceWorkerPath: undefined, skipDialog: displayArgs[5] } | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
export const enablePush = (logger, account, request, customSwPath, skipDialog) => { | ||||||||||||||||||||||||||||||||||||||
|
@@ -34,6 +80,10 @@ export const enablePush = (logger, account, request, customSwPath, skipDialog) = | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
if (customSwPath) { swPath = customSwPath } | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
if (skipDialog === null) { | ||||||||||||||||||||||||||||||||||||||
skipDialog = false | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
notificationHandler = new NotificationHandler({ logger, session: {}, request, account }) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
if (skipDialog) { | ||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are exposing new function in a public API, please check if it can be avoided, in case there are some contraints try adding them in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to segregate the functionality of handling old/new soft prompt in a new function in the file prompt.js.
In processSoftPrompt function which is in prompt.js we create an instance of notificationHandler class as we need to call setupwebpushnotification method as well as a private method #setupWebPush .
Since we can't access private methods I had to create a method setupWebPush in notification.js which can be accessed from prompt.js file. This method internally calls the private method this.#setupWebPush.
Even if I try to move the logic of rendering old/nwe soft prompt inside of a private method in notificationhandler class, there is a method processWebpushConfig which is called in tr.js and code is written in prompt.js. Then again there will be a problem to call this new private method which takes care of handling of old/new soft prompt from prompt.js as since it will be a private method in notification.js
We were anyway exposing push as a public method before too which calls this.#setupwebpush, I just created another public api setupwebpush which calls this.#setupWebpush.