Skip to content

Conversation

tinni95
Copy link

@tinni95 tinni95 commented Nov 13, 2020

Added the ability to pass a URL as the endpoint to get an authentication token to send and authenticate inside the chat

Copy link
Contributor

@fbartho fbartho left a comment

Choose a reason for hiding this comment

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

@tinni95 thanks for your contribution!

Unfortunately, I'm not super comfortable with this PR in it's current state.

  1. There are files and changes that shouldn't be there
  2. There are files missing, that appear to need to be there
  3. This completely changes how we initialize things for all users, rather than being an add on, that people can opt-into only if required.
  4. There's no documentation linked for me to understand what you're trying to accomplish, how Zendesk recommends you do this, or how to verify that it's working?

Please let me know if you'd like to remedy these issues!


api group: 'com.zendesk', name: 'chat', version: safeExtGet('zendeskChatVersion', '3.1.0')
api group: 'com.zendesk', name: 'messaging', version: safeExtGet('zendeskMessagingVersion', '5.1.0')
api group: 'com.zendesk', name: 'support', version: safeExtGet('zendeskMessagingVersion', '2.3.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
api group: 'com.zendesk', name: 'support', version: safeExtGet('zendeskMessagingVersion', '2.3.0')
api group: 'com.zendesk', name: 'support', version: safeExtGet('zendeskSupportVersion', '2.3.0')

@@ -0,0 +1,6 @@
#Thu Oct 29 18:09:13 CET 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super convinced these gradle/gradle-wrapper files are necessary for a library. Did you mean to commit them?

"repository": {
"type": "git",
"url": "https://github.com/taskrabbit/react-native-zendesk-chat"
"url": "https://github.com/tinni95/react-native-zendesk-chat"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"url": "https://github.com/tinni95/react-native-zendesk-chat"
"url": "https://github.com/taskrabbit/react-native-zendesk-chat"

[ZDKChat initializeWithAccountKey:zenDeskKey queue:dispatch_get_main_queue()];
}
ZDKJWTAuth *authenticator = [ZDKJWTAuth new];
[authenticator setUrl:appId];
Copy link
Contributor

Choose a reason for hiding this comment

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

appId isn't really a URL?

.withPhoneFieldStatus(getFieldStatusOrDefault(options, "phone", defaultValue))
.withDepartmentFieldStatus(getFieldStatusOrDefault(options, "department", defaultValue));
.withPhoneFieldStatus(getFieldStatusOrDefault(options, "phone", defaultValue));
//.withDepartmentFieldStatus(getFieldStatusOrDefault(options, "department", defaultValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental?

B6462EB61C603E340010294B = {
isa = PBXGroup;
children = (
A73FC0C32550745000B7EC05 /* jwtAuth.swift */,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding Swift to react-native libraries can cause problems for some customers. Was this the only way to accomplish the task?

SKIP_INSTALL = YES;
SWIFT_OBJC_BRIDGING_HEADER = "RNZendeskChat-Bridging-Header.h";
SWIFT_OPTIMIZATION_LEVEL = "-Onone";
SWIFT_VERSION = 5.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Our customers may not be using SWIFT_VERSION = 5.0

objects = {

/* Begin PBXBuildFile section */
A73FC0C42550745000B7EC05 /* jwtAuth.swift in Sources */ = {isa = PBXBuildFile; fileRef = A73FC0C32550745000B7EC05 /* jwtAuth.swift */; };
Copy link
Contributor

Choose a reason for hiding this comment

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

This file appears missing?

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.

2 participants