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

Display alert when an old, trial-only binding is added or backfilled #1440

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

Conversation

kiahna-tucker
Copy link
Member

@kiahna-tucker kiahna-tucker commented Jan 31, 2025

Issues

The issues directly below are completely resolved by this PR:
#1409

Changes

1409

The following features are included in this PR:

  • Extend binding store state to include a top-level, collection-specific metadata property (i.e., collectionMetadata). The property is a dictionary type that is keyed by the catalog name of the collection. The following pieces of metadata are currently stored there: added, sourceBackfillRecommended, trialOnlyStorage, and updatedAt.

  • Create a hook, useTrialPrefixes, to evaluate, store, and return prefixes that only have the trial storage mapping defined.

  • Create a hook, useTrialCollections, to evaluate and return metadata for collections under trial prefixes.

  • Display a warning alert in the binding-level Backfill section when the selected binding is over 20 days old, the corresponding collection is under a trial prefix, and it is backfilled in the current workflow.

  • Display a warning alert in the top-level Backfill section when the at least one backfilled binding is over 20 days old and the corresponding collection is under a trial prefix.

  • Display a warning alert in the right pane of the binding selector when the selected binding is over 20 days old, the corresponding collection is under a trial prefix, and it was added in the current workflow.

  • Store the numeric value of the trial duration of 20 days in an environment variable.

  • Create component for the alert indicator displayed in the CollectionConfig header.

Tests

Manually tested

Approaches to testing are as follows:

  • Validate that the indicator on the Source Collections section header displays in a warning state at least one binding is over 20 days old, the corresponding collection is under a trial prefix, and backfilled or added in the current workflow.

  • Validate that the indicator on a binding list item displays in a warning state when the binding is over 20 days old, the corresponding collection is under a trial prefix, and backfilled or added in the current workflow.

  • Validate that the error state of the indicator on a binding list item and the Source Collections header takes precedence over the warning state.

  • Validate that a warning alert displays in the top-level Backfill section when at least one binding is over 20 days old, the corresponding collection is under a trial prefix, and backfilled in the current workflow.

  • Validate that a warning alert displays in the binding-level Backfill section when the binding is over 20 days old, the corresponding collection is under a trial prefix, and backfilled in the current workflow.

  • Validate that a warning alert displays above the Resource Config header when the binding is over 20 days old, the corresponding collection is under a trial prefix, and added in the current workflow.

  • Validate that source backfill recommended alerts are preserved when the Next button is clicked and the browser refreshed.

Automated tests

N/A

Playwright tests ran locally

  • Admin
  • Captures
  • Collections
  • HomePage
  • Login
  • Materialization

Screenshots

Source Collections section header when section contains a source backfill recommended alert

pr_screenshot-1440-trial_collections-backfill-form_section_warning_indication

Binding list item when a backfill is recommended for its source

pr_screenshot-1440-trial_collections-backfill-selected_binding_warning_indication

Top-level alert | Backfill

pr_screenshot-1440-trial_collections-backfill-top_level_alert

Binding-level alert | Backfill

pr_screenshot-1440-trial_collections-backfill-binding_level_alert

Binding-level alert | Added

pr_screenshot-1440-trial_collections-added-binding_level_alert

@kiahna-tucker kiahna-tucker added the change:planned This is a planned change label Feb 7, 2025
@kiahna-tucker kiahna-tucker marked this pull request as ready for review February 10, 2025 20:49
@kiahna-tucker kiahna-tucker requested a review from a team as a code owner February 10, 2025 20:49
Comment on lines +393 to +399
? difference(
draftedBindings.map((binding) =>
getCollectionName(binding)
),
liveBindings.map((binding) => getCollectionName(binding))
)
: [],
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to change anything here... but we could maybe make this a bit eaiser on processing since if we are here we have already done through at least one of these arrays. However, it could end up getting more complex to "cache" off one of these lists while supporting multiple cases.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I guess we always know we have a drafterBindings here... so potentially worth it to make a shared array that is built up in the loop above. But not sure how that would impact performance with the set

@@ -112,6 +125,14 @@ function useUpdateBackfillCounter() {
backfilled && increment === 'false';

if (shouldIncrement || shouldDecrement) {
increment === 'true'
Copy link
Member

Choose a reason for hiding this comment

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

Since we need to handle this logic in two places... I think we could just store off which array to populate by storing the key up above before we start looping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:planned This is a planned change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants