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

chore:[#186705729] add query find user by business name #9283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aadedejifearless
Copy link
Contributor

@aadedejifearless aadedejifearless commented Dec 17, 2024

Description

This PR introduces a new feature to search for users by their business name using a query in AWS DynamoDB. The approach involves creating an SQL query statement in the DynamoUserDataClient that searches for users with businesses. The query checks for the existence of businesses and ensures that the businessName field is not missing.

Once the query is executed, the results are ingested into the DynamoDataClient. There, the business names are normalized (trimmed and converted to lowercase) to ensure case-insensitive matching when filtering for users with a business name that matches the search term.

Ticket

This pull request resolves #186705729.

Approach

  • Added queryUsersWithBusinesses in DynamoUserDataClient to execute a DynamoDB query that retrieves users with businesses.
  • Implemented findUserByBusinessName in DynamoDataClient, which filters users based on a normalized, case-insensitive business name.
  • Also updated the Router tests where the mock/stub DynamoDataClient lives as well as the Batch files where the mock/stub DynamoUserDataClient lives.

Steps to Test

  1. Run Unit Tests
    The following tests were created to verify the functionality of the changes here and previous existing logic in the DynamoDataClient.
  • should find user by businessName ...: This test ensures that the findUserByBusinessName method filters users correctly by matching business names, including case-insensitive matches.
  • should migrate data for multiple users with outdated versions...: This test verifies the migration of outdated user data, ensuring the business data is updated and logged appropriately
  1. Run the test suite with yarn test and ensure the tests pass successfully.

Notes

Code author checklist

  • I have rebased this branch from the latest main branch
  • I have performed a self-review of my code
  • I have created and/or updated relevant documentation on the engineering documentation website
  • I have not used any relative imports
  • I have pruned any instances of unused code
  • I have not added any markdown to labels, titles and button text in config
  • If I added/updated any values in userData (including profileData, formationData etc), then I added a new migration file
  • I have checked for and removed instances of unused config from CMS
  • If I added any new collections to the CMS config, then I updated the search tool and cmsCollections.ts (see CMS Additions in Engineering Reference/FAQ on the engineering documentation site)
  • I have updated relevant .env values in both .env-template and in Bitwarden

@aadedejifearless aadedejifearless force-pushed the ade_186705729AddQueryFindUserByBusinessName branch from 448dfb9 to 0a6e5af Compare December 17, 2024 19:52
@aadedejifearless aadedejifearless changed the title Ade 186705729 add query find user by business name chore:[#186705729] add query find user by business name Dec 17, 2024
@aadedejifearless aadedejifearless marked this pull request as ready for review December 17, 2024 19:54
Copy link
Member

@seidior seidior left a comment

Choose a reason for hiding this comment

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

Something I forgot to mention in the review of the last DB split ticket: is there anything in the new business structure that points back to the UUID of the user? This may make the work in this ticket a bit easier, since you in theory would just be able to query the secondary index for business names on the businesses table, get the business data, then have the UUID of its owner, and return that object.

api/src/db/DynamoUserDataClient.ts Outdated Show resolved Hide resolved
const normalizedBusinessName = businessName.trim().toLowerCase();
return usersWithBusinesses.filter((user: UserData) => {
return Object.values(user.businesses).some(
(business: Business) => business.profileData?.businessName?.toLowerCase() === normalizedBusinessName
Copy link
Member

Choose a reason for hiding this comment

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

[boulder] How will this work when business data is no longer inside user data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the workflow relies on business data being inside the user data structure. In the planned migration, we will remove the businesses object from userData and make slight updates to the code.

We will introduce a GSI (currentBusinessId), the common field between the users and businesses tables. The updated flow will:

  • Query the businessesTable to find businesses by name with fuzzy (matching: this is now implemented in the current state).
  • Extract the currentBusinessId from the matched business.
  • Query the usersTable using currentBusinessId to find the associated users.
    let me know your thoughts @seidior . this also answers your previous questions on what field do we have that ties both user and businesses currently.

Copy link
Member

Choose a reason for hiding this comment

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

To sum up what we discussed in our meeting just now:

  • The current migration for splitting out users and businesses and keeping them in sync should be modified (or a follow-up migration should be added) so that, in addition to a version field being added to the business record, we also add a field that is called something like "owningUser" that is the UUID of the user data record.
  • We then can modify this work to use the secondary index for business names on the businesses table to find the corresponding business record, then use this "owningUser" field in the business record to get the user data and return that.
    • This means we don't have to search through all businesses of all users for the correct user data, we just have to find the fuzzy-matching businesses and return those user data objects. This should vastly speed up the performance of this endpoint.

@aadedejifearless aadedejifearless force-pushed the ade_186705729AddQueryFindUserByBusinessName branch from 0a6e5af to b5e137a Compare December 17, 2024 20:22
@aadedejifearless aadedejifearless force-pushed the ade_186705729AddQueryFindUserByBusinessName branch from b5e137a to f6c2ca8 Compare December 18, 2024 18:27
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