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

BED-4299 -- Display members of custom asset groups on Group Management #623

Merged
merged 4 commits into from
May 22, 2024

Conversation

maffkipp
Copy link
Contributor

Description

GET api/v2/asset-groups/{asset_group_id}/members currently only queries the system_tags property to determine AG membership. This means any custom asset groups created through the API will not return any results via this endpoint, since membership in those groups is stored on the user_tags property instead.

This fix updates the query to check for the asset group tag in both the system_tags and user_tags field. I also added a check after the query to filter out nodes that do not have an exact match in the tag list, due to the potential for naming collisions with our StringContains query (for example, a custom asset group with tag test_tag_1 also pulling in nodes with tag test_tag).

Motivation and Context

If a user is creating custom asset groups via the API, we want them to be able to view and modify those asset groups via the Group Management page (or the api/v2/asset-groups/{asset_group_id}/members endpoint).

How Has This Been Tested?

  • Added integration tests to our graph query to make sure it returns custom groups
  • Manual testing locally by creating a custom asset group, adding members, and checking the endpoint/group management page

Screenshots (if appropriate):

Screenshot 2024-05-21 at 2 08 09 PM

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

@maffkipp maffkipp self-assigned this May 21, 2024
@maffkipp maffkipp marked this pull request as ready for review May 21, 2024 22:37
Copy link
Contributor

@urangel urangel left a comment

Choose a reason for hiding this comment

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

Good thinking filtering out substrings and thanks for adding a test!

@maffkipp maffkipp merged commit 2c3eaac into main May 22, 2024
3 checks passed
@maffkipp maffkipp deleted the BED-4299--ag-members-missing branch May 22, 2024 20:24
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants