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

[SDK-3289] Update miscellaneous references and code snippets for Swift SDK #5235

Merged

Conversation

jerielng
Copy link
Contributor

@jerielng jerielng commented Apr 12, 2023

Pull Request/Issue Resolution

Description of Change:

These changes include updates for the Swift SDK that are outside the Swift SDK platform integration guide, particularly with

Closes #ISSUE_NUMBER_HERE

Is this change associated with a Braze feature/product release?

  • Yes (12/15/2022)
  • No

✔️ Pull Request Checklist
  • Check that you haven't removed any images (replacing an image with an updated one of the same name is fine), as this breaks the French site
  • Check that all links work.
  • Ensure you have completed our Contributors License Agreement.
  • Tag @josh-mccrowell-braze and @KellieHawks as a reviewer when your work is done and ready to be reviewed for merge. Are you an internal product manager? Reference the internal reviewing chart to tag the appropriate reviewer.
  • Tag others as reviewers as necessary.
  • If you have modified any links, be sure to add redirects to assets > js > broken_redirect_list.js
⭐ Helpful Wiki Shortcuts
❗ ATTN: For PR Reviewers
  • Read our Reviewing a PR page for more on our reviewing suggestions.
  • Read our Previewing Documentation page to see how to check the deployment.
    • Preview all changes in the linked Vercel environment by clicking the preview link in the vercel-bot comment in your PR.
❗ ATTN: Internal Reviewing Chart
Work at Braze and not sure who to tag for review?
Before tagging @josh-mccrowell-braze or @KellieHawks for a general review, reference the following chart to see if a specific product vertical/reviewer applies to your pull request.

Reviewer Product Vertical
@josh-mccrowell-braze Monolith Deployments
Quality Infrastructure
Platform Infrastructure
Datalake
SDKs
@KellieHawks Currents
Internal Tools
Product Partnerships
SMS
Customer Lifecycle, Identity and Permissions
@bre-fitzgerald Reporting
Intelligence
User Targeting
IAM
Channels
FIX
@lydia-xie Ingestion
Core Objects
Core Messaging
Messaging and Automation
Email (Composition and Infrastructure)

@cla-bot cla-bot bot added the cla-signed label Apr 12, 2023
@vercel
Copy link

vercel bot commented Apr 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
braze-docs-en ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 20, 2023 7:02pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
braze-docs-fr ⬜️ Ignored (Inspect) Apr 20, 2023 7:02pm

@jerielng jerielng force-pushed the feature/sdk-3289-swift-misc branch from 0a43b07 to 56cf033 Compare April 12, 2023 16:52
@@ -293,30 +292,44 @@ Braze.getInstance(this).subscribeToSdkAuthenticationFailures({ error: BrazeSdkAu
```
{% endtab %}
{% tab Objective-C %}

{% alert important %}
Starting in version `5.14.0` of the Braze Swift SDK, the SDK authentication delegate method has been split from the `BrazeDelegate` into a separate `BrazeSDKAuthDelegate`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is pending release, so we'll need to coincide this merge with the release of the new version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jerielng Do you know the time frame of the release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll be within the next week. We have most of the changes ready, just doing some final merges and reviews.

@@ -77,7 +77,7 @@ To learn more about JSON Web Tokens, or to browse the many open source libraries

This feature is available as of the following [SDK versions]({{ site.baseurl }}/user_guide/engagement_tools/campaigns/ideas_and_strategies/new_features/#filtering-by-most-recent-app-versions):

{% sdk_min_versions web:3.3.0 ios:4.3.0 android:14.0.0 %}
{% sdk_min_versions web:3.3.0 ios:5.0.1 android:14.0.0 %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This linking seems to be broken for some reason. I'm not sure what's going on here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @josh-mccrowell-braze can take a look

Copy link
Contributor

Choose a reason for hiding this comment

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

One complication is that this feature is supported on:

  • AppboyKit 4.3.0+
  • Swift SDK 5.0.1+

Since this is removing our public docs for AppboyKit, I'm debating if we are OK with:

  1. Completely removing AppboyKit docs
  2. Removing AppboyKit docs but linking out to the sample AppboyKit code integration
  3. Keeping a copy of both AppboyKit docs and Swift SDK docs

I'm leaning towards 2) where at the bottom of the ObjC and Swift sections we link out to the sample code in Stopwatch, even though it's in ObjC. This would be relevant for Enable this feature in the Braze SDK and Register a callback function for invalid tokens sections:

For the legacy AppboyKit SDK, reference [this file]({file_here}) for sample code.

@jerielng jerielng changed the title [WIP][SDK-3289] Update miscellaneous references and code snippets for Swift SDK [SDK-3289] Update miscellaneous references and code snippets for Swift SDK Apr 12, 2023
@jerielng jerielng marked this pull request as ready for review April 12, 2023 17:16
Copy link
Contributor

@hokstuff hokstuff left a comment

Choose a reason for hiding this comment

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

Looks good overall - just one main point about having some links to the old AppboyKit SDK Auth integration

@@ -77,7 +77,7 @@ To learn more about JSON Web Tokens, or to browse the many open source libraries

This feature is available as of the following [SDK versions]({{ site.baseurl }}/user_guide/engagement_tools/campaigns/ideas_and_strategies/new_features/#filtering-by-most-recent-app-versions):

{% sdk_min_versions web:3.3.0 ios:4.3.0 android:14.0.0 %}
{% sdk_min_versions web:3.3.0 ios:5.0.1 android:14.0.0 %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @josh-mccrowell-braze can take a look

@@ -77,7 +77,7 @@ To learn more about JSON Web Tokens, or to browse the many open source libraries

This feature is available as of the following [SDK versions]({{ site.baseurl }}/user_guide/engagement_tools/campaigns/ideas_and_strategies/new_features/#filtering-by-most-recent-app-versions):

{% sdk_min_versions web:3.3.0 ios:4.3.0 android:14.0.0 %}
{% sdk_min_versions web:3.3.0 ios:5.0.1 android:14.0.0 %}
Copy link
Contributor

Choose a reason for hiding this comment

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

One complication is that this feature is supported on:

  • AppboyKit 4.3.0+
  • Swift SDK 5.0.1+

Since this is removing our public docs for AppboyKit, I'm debating if we are OK with:

  1. Completely removing AppboyKit docs
  2. Removing AppboyKit docs but linking out to the sample AppboyKit code integration
  3. Keeping a copy of both AppboyKit docs and Swift SDK docs

I'm leaning towards 2) where at the bottom of the ObjC and Swift sections we link out to the sample code in Stopwatch, even though it's in ObjC. This would be relevant for Enable this feature in the Braze SDK and Register a callback function for invalid tokens sections:

For the legacy AppboyKit SDK, reference [this file]({file_here}) for sample code.

@@ -58,23 +58,24 @@ These properties are collected by the Android SDK upon proper integration.
{: .reset-td-br-1 .reset-td-br-2}
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 is named _docs/_developer_guide/platform_integration_guides/sdk_primer.md but the title on top is SDK 101 and the content doesn't mention push primers - do we want to rename this file (+ possible redirects) @josh-mccrowell-braze?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. The only thing that the name of the file affects (that I know of, anyway) is the URL, which is sort of low-value compared to the article title and nav title. Does the need to update the URL outweigh the risks? There's a few instances of this here and there and I generally think the update is low value.

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 any concern with updating the name of the file (does it break anything)? I think if it's easy and doesn't interfere with anything, it might be fine to just update the file name so it doesn't stay confusing in the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I realize now that we've talked about this that the reason this is displaying this way (although the file name is still the same) is because the feature branch is outdated. That article has gotten some TLC in the meantime.

I think it's fair to change the name of the file, and will take a note to do so, but will implement on another branch.

Copy link
Contributor

@lowip lowip left a comment

Choose a reason for hiding this comment

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

LGTM after the changes related to AppboyKit

@jerielng jerielng force-pushed the feature/sdk-3289-swift-misc branch from 33d7394 to d4d0a00 Compare April 14, 2023 18:30
@jerielng
Copy link
Contributor Author

@braze-inc/sdks-web Sanity checking here, is this code snippet correct? Noticed inside the closure, it's referencing appboy.setSdkAuthenticationSignature(updated_jwt);

import * as braze from"@braze/web-sdk";
braze.subscribeToSdkAuthenticationFailures((error) => {
  // TODO: Optionally log to your error-reporting service
  // TODO: Check if the `user_id` within the `error` matches the currently logged-in user
  const updated_jwt = await getNewTokenSomehow(error);
  appboy.setSdkAuthenticationSignature(updated_jwt);
});

@jerielng
Copy link
Contributor Author

@braze-inc/sdks-android Sanity check for the Java and Kotlin tabs for this section here. Seems to be referencing appboy.changeUser and pointing to this documentation link. Should these be updated?

endpoint: "{YOUR-BRAZE-ENDPOINT}")
configuration.api.sdkAuthentication = true
let braze = Braze(configuration: configuration)
AppDelegate.braze = braze
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Bump for this comment about including a way to access the example code for AppboyKit if they are using the legacy SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included it here at the top-level instead of at each code snippet - let me know what you think!

@josh-mccrowell-braze
Copy link
Collaborator

Just FYI, re: the sdk_min_version not linking correctly, that has been fixed here: #5262

Co-authored-by: Daniel Hok <4797040+hokstuff@users.noreply.github.com>
@josh-mccrowell-braze josh-mccrowell-braze merged commit 3153737 into feature/swift-sdk-docs-epic Apr 20, 2023
@josh-mccrowell-braze josh-mccrowell-braze deleted the feature/sdk-3289-swift-misc branch April 20, 2023 19:04
@jerielng jerielng mentioned this pull request May 12, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants