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

DOP-3870 Adds the Chatbot UI wrapper around the Chatbot package and ensure it is render via CF. #871

Merged
merged 28 commits into from
Aug 30, 2023

Conversation

caesarbell
Copy link
Collaborator

@caesarbell caesarbell commented Aug 24, 2023

Stories/Links:

DOP-3870

Current Behavior:

Production

Staging Links:

Comment Stage

Notes:

We are currently facing some CORS issues. I am working with them to add the following URLs to their allowed list

We can actually push forward with the React.Lazy loading and not have it wrapped within a Feature Flag. After reading some articles and the documentation it shouldn't have any side-effect if the underlying component can be used on the server or client.

We can also view it on the chat server staging URL

image image

Copy link
Collaborator

@rayangler rayangler left a comment

Choose a reason for hiding this comment

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

Hey, @caesarbell, is it okay if you add a staging link to this PR please?

src/components/ChatbotUi.js Outdated Show resolved Hide resolved
src/components/ChatbotUi.js Show resolved Hide resolved
@caesarbell
Copy link
Collaborator Author

The staging link has been added.

src/components/ChatbotUi.js Show resolved Hide resolved
src/components/ChatbotUi.js Show resolved Hide resolved
</StyledLoadingSkeletonContainer>
}
>
<LazyChatbot serverBaseUrl="https://chat-server.docs.staging.corp.mongodb.com/api/v1" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove this link to preserve default server behavior?

Copy link
Collaborator Author

@caesarbell caesarbell Aug 29, 2023

Choose a reason for hiding this comment

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

@rayangler we may need to lean on an environment variable to correctly provide this prop with the URL pointing to their staging server. I don't think currently it is being handled within the chatbot itself. I was told the URLs we provided them were added to their allowed list for their staging server only.

Copy link
Collaborator

@rayangler rayangler Aug 30, 2023

Choose a reason for hiding this comment

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

I'm okay with the env variable, but is there a benefit in having our staging sites only be able to access their staging server? I imagine it would be more beneficial for our component to access their production server so that we can be confident our site's component will work properly with the chatbot before it's deployed onto the MongoDB site.

Additionally, staging instances hosted on Kanopy require machine-machine authentication through client credentials. I'm not sure if there's a route that we could take on our end to ensure that communication is successful (currently, based on the staging link for this PR).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. Okay, I was unaware of how our staging setup is for Kanopy. It probably is more beneficial to have the component talk to their prod server. As you said above it will also increase our confidence in knowing everything works before deploying to prod.

Copy link
Collaborator

@rayangler rayangler Aug 30, 2023

Choose a reason for hiding this comment

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

We should reach out to the chatbot team to allow our sites to hit their production server to fix the ongoing issue of not being able to actually test the chatbot on our site. Or if we need to use some staging endpoint, the component should have some way to successfully authenticate. As-is, the component currently throws an error regardless of which endpoint we use

Copy link
Collaborator

@rayangler rayangler left a comment

Choose a reason for hiding this comment

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

Implementation of the component on our end lgtm. I'll leave it up to you if you want to merge this in now despite the chat server being inaccessible, or if we should wait until we can ensure the actual chatbot works on our site. Either one should be fine as long as we don't have docs-landing use the directive to render the chatbot component just yet

@caesarbell
Copy link
Collaborator Author

Implementation of the component on our end lgtm. I'll leave it up to you if you want to merge this in now despite the chat server being inaccessible, or if we should wait until we can ensure the actual chatbot works on our site. Either one should be fine as long as we don't have docs-landing use the directive to render the chatbot component just yet

As of now, there is no directive for it, so it won't render. But once we have a directive for it we will need to ensure this is working and passing CORs.

I will post in the channel and ask the leads what do they think. I don't want to make the wrong call.

@caesarbell caesarbell merged commit f4c5a57 into master Aug 30, 2023
2 checks passed
@caesarbell caesarbell deleted the DOP-3870 branch August 30, 2023 19:11
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