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

feat: explicitly log authentication errors as warnings #72

Conversation

luca-gr4vy
Copy link
Contributor

Description: Adds gr4vyId to the options object passed to the embed, for debugging purposes.

Ticket: https://gr4vy.atlassian.net/browse/TA-1026

Screenshot(s):

Screenshot 2022-03-21 at 17 28 41

@luca-gr4vy luca-gr4vy changed the title Feature/ta 1026 explicitly log authentication errors as warnings feat: Explicitly log authentication errors as warnings Mar 22, 2022
@luca-gr4vy luca-gr4vy changed the title feat: Explicitly log authentication errors as warnings feat: explicitly log authentication errors as warnings Mar 22, 2022
@luca-gr4vy luca-gr4vy self-assigned this Mar 22, 2022
@luca-gr4vy luca-gr4vy force-pushed the feature/TA-1026-explicitly-log-authentication-errors-as-warnings branch from 0971d64 to 2303a36 Compare March 22, 2022 16:02
@cbetta
Copy link
Member

cbetta commented Mar 22, 2022

@luca-gr4vy I think this works, but doesn't EmbedUI know the ID from the host it's loaded on? Not sure what makes more sense.

Copy link
Collaborator

@douglaseggleton douglaseggleton 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 to me :-) Before you hit merge though - See https://github.com/gr4vy/gr4vy-embed/blob/main/CONTRIBUTING.md#releasing-a-pr (I think internal is the correct label)

@luca-gr4vy
Copy link
Contributor Author

@luca-gr4vy I think this works, but doesn't EmbedUI know the ID from the host it's loaded on? Not sure what makes more sense.

@cbetta the following is taken from the dashboard codebase (it's how the Integrations page shows the id)

const getGr4vyId = (): string => {
  if (isDev()) {
    return 'dev'
  } else if (isSandbox()) {
    return document.location.host.split('.')[1]
  }
  return document.location.host.split('.')?.[0]
}

though, I believe getting it directly from the embed's config should be more robust (?)

@luca-gr4vy luca-gr4vy added the internal Changes only affect the internal API label Mar 23, 2022
@cbetta
Copy link
Member

cbetta commented Mar 24, 2022

@luca-gr4vy I think this works, but doesn't EmbedUI know the ID from the host it's loaded on? Not sure what makes more sense.

@cbetta the following is taken from the dashboard codebase (it's how the Integrations page shows the id)

const getGr4vyId = (): string => {
  if (isDev()) {
    return 'dev'
  } else if (isSandbox()) {
    return document.location.host.split('.')[1]
  }
  return document.location.host.split('.')?.[0]
}

though, I believe getting it directly from the embed's config should be more robust (?)

Agreed. Keep it as-is.

@luca-gr4vy luca-gr4vy merged commit a2a5afb into main Mar 24, 2022
@luca-gr4vy luca-gr4vy deleted the feature/TA-1026-explicitly-log-authentication-errors-as-warnings branch March 24, 2022 13:39
@gr4vy-code
Copy link
Collaborator

🚀 PR was released in v2.9.0 🚀

@gr4vy-code gr4vy-code added the released Issue or pull request released label Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes only affect the internal API released Issue or pull request released
Development

Successfully merging this pull request may close these issues.

None yet

4 participants