-
Notifications
You must be signed in to change notification settings - Fork 39
[MOB-12298] Improve the logger #739
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
Conversation
…gger functionality
…' into jwt/MOB-12298-new-improve-logger
|
Diff Coverage: The code coverage on the diff in this pull request is 93.9%. Total Coverage: This PR will increase coverage by 4.67%. File Coverage Changes
🛟 Help
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
…' into jwt/MOB-12298-new-improve-logger
…' into jwt/MOB-12298-new-improve-logger
…' into jwt/MOB-12298-new-improve-logger
…' into jwt/MOB-12298-new-improve-logger
…e/react-native-sdk into jwt/MOB-12298-new-improve-logger
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.
Made two comments.
- The two config values for log
- Default state of logging
| test('should have default logging enabled', () => { | ||
| expect(IterableLogger.loggingEnabled).toBe(true); | ||
| }); | ||
|
|
||
| test('should have default log level as info', () => { | ||
| expect(IterableLogger.logLevel).toBe(IterableLogLevel.info); | ||
| }); |
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.
Default should be debug. Info would add lot many logs and can be noisy
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.
Yeah that's totally fair. Changed it.
| if (config) { | ||
| Iterable.savedConfig = config; | ||
|
|
||
| IterableLogger.setLoggingEnabled(config.logReactNativeSdkCalls ?? true); |
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.
So whats the two log config?
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.
The two config values that are set are:
- Whether anything is going to be logged at all (
config.logReactNativeSdkCalls-->IterableLogger.loggingEnabled) - If things are being logged, how granular those logs are going to be (
config.logLevel-->IterableLogger.logLevel)
I think that's what you were asking. Let me know if I misinterpreted the question.
| return await Iterable.inAppManager?.getMessages().then((messagesObtained) => { | ||
| expect(messagesObtained).toEqual(messages); | ||
| }); | ||
| return await Iterable.inAppManager | ||
| ?.getMessages() | ||
| .then((messagesObtained) => { | ||
| expect(messagesObtained).toEqual(messages); | ||
| }); | ||
| }); | ||
|
|
||
| test('showMessage_messageAndConsume_returnsClickedUrl', async () => { | ||
| // GIVEN an in-app message and a clicked url | ||
| const messageDict = { | ||
| messageId: 'message1', | ||
| campaignId: 1234, | ||
| trigger: { type: IterableInAppTriggerType.immediate }, | ||
| }; | ||
| const message: IterableInAppMessage = | ||
| IterableInAppMessage.fromDict(messageDict); | ||
| const consume: boolean = true; | ||
| const clickedUrl: string = 'testUrl'; | ||
|
|
||
| // WHEN the simulated clicked url is set to the clicked url | ||
| MockRNIterableAPI.setClickedUrl(clickedUrl); | ||
| // THEN Iterable,inAppManager.showMessage returns the simulated clicked url | ||
| return await Iterable.inAppManager?.showMessage(message, consume).then((url) => { | ||
| expect(url).toEqual(clickedUrl); | ||
| }); | ||
| return await Iterable.inAppManager | ||
| ?.showMessage(message, consume) | ||
| .then((url) => { | ||
| expect(url).toEqual(clickedUrl); | ||
| }); | ||
| }); |
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.
Not logging related as such.
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.
This was automatically done by my automatic formatter :/
@Ayyanchira Responded to these! |
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.
Looks good :)
🔹 JIRA Ticket(s) if any
✏️ Description
Changed the methods on the logger to be static, minimizing the need for circular dependencies.
Testing
config.logReactNativeSdkCalls = true. Make sure the logs show.config.logReactNativeSdkCalls = false. Make sure the logs do not show.