Add Bot State API deprecation warning #348

nwhitmont · 2017-11-17T19:20:42Z

No description provided.

carlosscastro

Nice Nils!!! Just 2 minor comments on the text, the rest looks good to me (though I'm no emulator expert)

carlosscastro · 2017-11-17T19:23:57Z

src/server/controllers/connector/botStateController.ts

+    private logBotStateApiDepreciationWarning(botId: string, conversationId: string) {
+        const conversation: Conversation = emulator.conversations.conversationById(botId, conversationId);
+        if (!conversation.stateApiDepretiationWarningShown) {
+            log.warn('The Bot State API has been depreciated. Please configure your own Bot State API. For more information see: https://blog.botframework.com/2017/07/21/saving-state-azure-nodejs/')


maybe "is being" instead of "has been" deprecated?

The link should probably be to the documentation here:
C#: https://docs.microsoft.com/en-us/bot-framework/dotnet/bot-builder-dotnet-state-azure-table-storage
Node: https://docs.microsoft.com/en-us/bot-framework/nodejs/bot-builder-nodejs-state-azure-table-storage

The links look kind of long but I like pointing to the docs. And also we should not make assumptions on whether they use c# or node so probably we need to link both

I created aka.ms URLs for both documentation links, and added ccastro && eanders as co-owners.

aka.ms/bot-stateapi-dotnet

aka.ms/bot-stateapi-nodejs

nwhitmont · 2017-11-17T19:51:06Z

@carlosscastro @eanders-ms

Warning text updated.
docs links replaced with aka.ms URLs

eanders-ms · 2017-11-17T20:33:55Z

src/server/controllers/connector/botStateController.ts

@@ -56,14 +58,23 @@ export class BotStateController {
        return `${botId || '*'}!${channelId || '*'}!${conversationId || '*'}!${userId || '*'}`;
    }

+    private logBotStateApiDepreciationWarning(botId: string, conversationId: string) {
+        const conversation: Conversation = emulator.conversations.conversationById(botId, conversationId);
+        if (!conversation.stateApiDepreciationWarningShown) {


Check for undefined conversation

eanders-ms · 2017-11-17T20:34:26Z

src/server/controllers/connector/botStateController.ts

@@ -56,14 +58,23 @@ export class BotStateController {
        return `${botId || '*'}!${channelId || '*'}!${conversationId || '*'}!${userId || '*'}`;
    }

+    private logBotStateApiDepreciationWarning(botId: string, conversationId: string) {
+        const conversation: Conversation = emulator.conversations.conversationById(botId, conversationId);
+        if (!conversation.stateApiDepreciationWarningShown) {


Where does stateApiDepreciationWarningShown get set to true?

botStateController.ts L64

eanders-ms · 2017-11-17T20:35:07Z

src/server/controllers/connector/botStateController.ts

+    private logBotStateApiDepreciationWarning(botId: string, conversationId: string) {
+        const conversation: Conversation = emulator.conversations.conversationById(botId, conversationId);
+        if (!conversation.stateApiDepreciationWarningShown) {
+            log.warn('The Bot State API is being depreciated. Please configure your own Bot State API. For more information: .NET https://aka.ms/bot-stateapi-dotnet | Node.js https://aka.ms/bot-git stateapi-nodejs')


This word should be deprecated, not depreciated.

The links in the message should be hyperlinked. Example how to do it:

log.warn(log.makeLinkMessage('Read about the Bot Framework authentication change.', 'https://aka.ms/botfxv32authchange'));

Also you can search the codebase for additional examples.

eanders-ms · 2017-11-17T20:35:32Z

src/server/conversationManager.ts

+    // flag indicating if the user has been shown the 
+    // "please don't use default Bot State API" warning message
+    // when they try to write bot state data
+    public stateApiDepreciationWarningShown: boolean = false;


Rename to stateApiDeprecationWarningShown

eanders-ms

Please see comments

nwhitmont · 2017-11-17T22:33:57Z

updated

nwhitmont · 2017-11-17T22:36:25Z

changed "depreciated" to "deprecated" in function and variable names

eanders-ms · 2017-11-17T23:17:16Z

src/server/controllers/connector/botStateController.ts

+        } else {
+            if (!conversation.stateApiDeprecationWarningShown) {
+                conversation.stateApiDeprecationWarningShown = true;
+                log.warn('The Bot State API is being depreciated. Please configure your own Bot State API. For more information: .NET https://aka.ms/bot-stateapi-dotnet | Node.js https://aka.ms/bot-stateapi-nodejs')


Please hyperlink urls in the message (see my other comment for sample code).

converted to single URL --> https://aka.ms/botframework-state-service

nwhitmont · 2017-11-28T18:41:09Z

@eanders-ms warning message has been hyperlinked per feedback.

eanders-ms · 2017-11-28T18:52:58Z

@nwhitmont, Can you please update the warning with this text:

Warning: The Bot Framework State API is not recommended for production environments, and may be deprecated in a future release. Learn how to implement your own storage adapter.

where

Learn how to implement your own storage adapter.

is the hyperlinked part.

Thank you!

eanders-ms · 2017-11-29T05:06:11Z

src/server/controllers/connector/botStateController.ts

+    private logBotStateApiDeprecationWarning(botId: string, conversationId: string) {
+        const conversation: Conversation = emulator.conversations.conversationById(botId, conversationId);
+        if (!conversation) {
+            log.error('Error loading conversation object');


Please remove this line. A missing conversationId value isn't an error. It is valid when the bot is getting/setting user data.

For now, don't show the warning if you don't have a conversationId.

eanders-ms · 2017-11-29T19:43:42Z

src/server/controllers/connector/botStateController.ts

-            }
-        }
-
+        if (!conversation.stateApiDeprecationWarningShown) {


This code will crash when reading user data. Did you test locally before committing?

eanders-ms · 2017-11-29T21:40:03Z

src/server/controllers/connector/botStateController.ts

-            conversation.stateApiDeprecationWarningShown = true;
-            log.warn('Warning: The Bot Framework State API is not recommended for production environments, and may be deprecated in a future release.',
-            log.makeLinkMessage('Learn how to implement your own storage adapter.', 'https://aka.ms/botframework-state-service'));
+        if (conversation) {


Can you please simplify this to a single line?

Since there aren't any else statements in this method, what about:

if (conversation && !conversation.stateApiDeprecationWarningShown) {

as opposed to

if (conversation) { if (!conversation.stateApiDeprecationWarningShown) {

eanders-ms · 2017-11-30T18:05:42Z

src/server/controllers/connector/botStateController.ts

@@ -56,14 +58,26 @@ export class BotStateController {
        return `${botId || '*'}!${channelId || '*'}!${conversationId || '*'}!${userId || '*'}`;
    }

+    private logBotStateApiDeprecationWarning(botId: string, conversationId: string) {
+        const conversation: Conversation = emulator.conversations.conversationById(botId, conversationId);
+        if (conversation) { if (!conversation.stateApiDeprecationWarningShown) {


Oh, sorry I wasn't more clear. I was asking you to combine the two statements into a single statement:
if (conversation && !conversation.stateApiDeprecationWarningShown) { ...

Add Bot State API depreciation warning

8393454

nwhitmont added the Enhancement New feature or request to evolve an additional feature label Nov 17, 2017

nwhitmont self-assigned this Nov 17, 2017

nwhitmont requested review from eanders-ms, tomlm and carlosscastro November 17, 2017 19:20

carlosscastro suggested changes Nov 17, 2017

View reviewed changes

update warning message text + replace docs URLs with aka.ms links

fac6c5c

Nils Whitmont added 4 commits November 17, 2017 12:02

fix typo in docs link

24d87e6

src/server/controllers/connector/botStateController.ts

22a20d8

update variable name

3a3103f

fix typo

1846284

eanders-ms reviewed Nov 17, 2017

View reviewed changes

eanders-ms suggested changes Nov 17, 2017

View reviewed changes

danmarshall changed the title ~~Add Bot State API depreciation warning~~ Add Bot State API deprecation warning Nov 17, 2017

set depreciation warning flag to true after first warning.

651a858

nwhitmont removed the request for review from tomlm November 17, 2017 22:33

change "depreciation" to "deprecation" in method and variable name

14fb026

add check for conversation object

bca9fe8

eanders-ms reviewed Nov 17, 2017

View reviewed changes

hyperlink log message

8df0c98

eanders-ms removed the Enhancement New feature or request to evolve an additional feature label Nov 28, 2017

Nils Whitmont added 2 commits November 28, 2017 13:54

updated warning message per eanderson feedback

5b744d1

split messages linked/not linked

fa5f143

eanders-ms reviewed Nov 29, 2017

View reviewed changes

update per eanders feedback

9621eac

eanders-ms reviewed Nov 29, 2017

View reviewed changes

update per eanders feedback

7e3e390

eanders-ms reviewed Nov 29, 2017

View reviewed changes

update per eanders feedback

06a1f88

eanders-ms reviewed Nov 30, 2017

View reviewed changes

update per eanders feedback

017b2c9

eanders-ms approved these changes Nov 30, 2017

View reviewed changes

eanders-ms merged commit e1ab20b into master Nov 30, 2017

eanders-ms deleted the nwhitmont/add-bot-state-api-depreciation-warning branch November 30, 2017 20:32

natelandon mentioned this pull request Oct 5, 2022

[Snyk] Fix for 19 vulnerabilities natelandon/BotFramework-Emulator#17

Open

This was referenced Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities 47-studio-org/BotFramework-Emulator#13

Open

[Snyk] Fix for 1 vulnerabilities MarcelRaschke/BotFramework-Emulator#16

Open

snyk-bot mentioned this pull request Nov 7, 2022

[Snyk] Fix for 1 vulnerabilities MarcelRaschke/BotFramework-Emulator#17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Bot State API deprecation warning #348

Add Bot State API deprecation warning #348

nwhitmont commented Nov 17, 2017

carlosscastro left a comment

carlosscastro Nov 17, 2017

carlosscastro Nov 17, 2017

carlosscastro Nov 17, 2017

nwhitmont Nov 17, 2017

nwhitmont commented Nov 17, 2017

eanders-ms Nov 17, 2017

nwhitmont Nov 17, 2017

eanders-ms Nov 17, 2017

nwhitmont Nov 17, 2017

nwhitmont Nov 17, 2017

eanders-ms Nov 17, 2017

eanders-ms Nov 17, 2017

eanders-ms Nov 17, 2017

nwhitmont Nov 17, 2017

eanders-ms left a comment

nwhitmont commented Nov 17, 2017

nwhitmont commented Nov 17, 2017

eanders-ms Nov 17, 2017

nwhitmont Nov 27, 2017

nwhitmont Nov 27, 2017

nwhitmont commented Nov 28, 2017

eanders-ms commented Nov 28, 2017

eanders-ms Nov 29, 2017

eanders-ms Nov 29, 2017

eanders-ms Nov 29, 2017

eanders-ms Nov 29, 2017

nwhitmont Nov 30, 2017

stevengum Nov 30, 2017 •

edited

Loading

eanders-ms Nov 30, 2017

Add Bot State API deprecation warning #348

Add Bot State API deprecation warning #348

Conversation

nwhitmont commented Nov 17, 2017

carlosscastro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwhitmont commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders-ms left a comment

Choose a reason for hiding this comment

nwhitmont commented Nov 17, 2017

nwhitmont commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwhitmont commented Nov 28, 2017

eanders-ms commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengum Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengum Nov 30, 2017 •

edited

Loading