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

Relationship field - make sure selected value correctly displays #15553

Merged
merged 10 commits into from
Feb 17, 2025

Conversation

mike12345567
Copy link
Collaborator

Description

If the selected value of a relationship field is not within the first 100 elements then the contents will not be fetched correctly. This PR updates the component to Typescript (as well as performing a very basic conversion of some of the ancillary components in the area) and fetches if the value is not found in the options list.

Fixes: https://linear.app/budibase/issue/BUDI-8998/saved-value-does-not-show-in-select-picker-if-the-value-is-not-in-the
#15400

@mike12345567 mike12345567 self-assigned this Feb 14, 2025
Copy link

qa-wolf bot commented Feb 14, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/m labels Feb 14, 2025
if (touched) {
// @ts-expect-error updateProp doesn't appear to exist, needs investigation
Copy link
Member

Choose a reason for hiding this comment

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

updateProp is in the builder store - it's just not typescript at the moment (although Adri has a PR up), I assume that's why it's not happy? But yea it's there at line 41 in the builder store.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah this is likely the issue - I'll keep note for once this is merged/sorted out!

export let filter: SearchFilter[]
// not really obvious how to type this - some components pass other things here
// but it looks like the component data fetch should only work with tables
export let datasourceType: "table" = "table"
Copy link
Member

Choose a reason for hiding this comment

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

I think this was added to support the other BB reference types. For example in BBReferenceField we pass in datasourceType="user", and looking at the possible types I think this could either be table, user or groupUser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this causes type errors on line 37 with:

  $: fetch = fetchData({
    API,
    datasource: {
      type: datasourceType,
      tableId: linkedTableId,
    },
    options: {
      filter,
      limit: 100,
    },
  })

This seems to think the only valid datasourceType is "table" - I didn't want to fiddle with the typing of data fetching, but something doesn't seem quite right there.

Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

This is a disgusting file hacked by too many people, so well done for successfully making a change - it's not easy. Maybe we'll be able to fully rewrite it at some point, and having TS will make that much easier so thank you!

Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

LGTM! I don't have a datasource capable of testing this in the product, but code changes look good 👍

@mike12345567 mike12345567 merged commit 1e979e4 into master Feb 17, 2025
20 checks passed
@mike12345567 mike12345567 deleted the fix/relationship-select branch February 17, 2025 14:51
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firestorm Data/Infra/Revenue Team size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants