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: support asset selection by external id #390

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mixu3
Copy link
Collaborator

@mixu3 mixu3 commented Jan 15, 2025

What this PR does / why we need it:

Currently, the user is required to input only the asset id in uuid format in order to select the asset in a query. If any user tries to enter an external id, the query will not be able to find timeseries data since the dataplane does not support external ids yet.

This change allows users to select assets by external ids and retrieve the timeseries data.

Screen.Recording.2025-01-15.at.1.16.41.PM.mov

In addition, a tooltip for the asset field teaches users how to enter the asset id. Users can learn more about SiteWise external ids by clicking on the API docs button.

AssetTooltip

Which issue(s) this PR fixes:

Fixes #351

Testing:

  • Unit tests passing
  • Manually testing these steps:
    • Typing in externalId:... retrieves the correct asset
    • Typing in the assetId in uuid format retrieves the correct asset (no change from previous behavior)
    • Typing in a string not in uuid format appends the externalId: prefix to retrieve the correct asset
    • Running queries on all three user inputs above loads the correct data
    • Successfully loads asset in query variable

@mixu3 mixu3 requested a review from a team as a code owner January 15, 2025 21:03
@mixu3 mixu3 requested review from iwysiu and idastambuk and removed request for a team January 15, 2025 21:03
@CLAassistant
Copy link

CLAassistant commented Jan 15, 2025

CLA assistant check
All committers have signed the CLA.

@mixu3 mixu3 requested review from chejimmy and hwandersman January 15, 2025 21:39
@hwandersman
Copy link
Collaborator

Things to consider to ensure customers will understand this change:

  1. Label on the asset dropdown - should it still always be "Asset ID"?
image
  1. Info icon on the Asset section with a description about how to select or enter an assetId. See an example for "Property Alias" here:
image

@mixu3
Copy link
Collaborator Author

mixu3 commented Jan 21, 2025

Things to consider to ensure customers will understand this change:

  1. Label on the asset dropdown - should it still always be "Asset ID"?
    ...
  2. Info icon on the Asset section with a description about how to select or enter an assetId.
    ...

After talking with UX, we agreed that we will keep the label as "Asset ID" but add an info icon to tell users how to enter the ID (UUID format or appending the prefix externalId:).

onChange({ ...query, assetIds: undefined });
} else {
const assetIds =
uuidRegex.test(assetId) || assetId.startsWith('externalId:') ? [assetId] : [`externalId:${assetId}`];
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested this, but I assume that if the user is using a template variable, i.e $myAssetId, then this will append the externalId to it, which we don't want. The easiest solution to this would probably be to add a check for startsWith($)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Support for SiteWise externalId on asset search
5 participants