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

fix: import duplicate features #4550

Merged
merged 1 commit into from
Aug 23, 2023
Merged

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Aug 23, 2023

About the changes

  • Import should fail when we have same feature twice
Screenshot 2023-08-23 at 10 49 24
  • fixing typechecker errors in all files that I touched
  • added create or update semantics for feature toggles, previously we only had create
  • improved feature toggle store to fail fast
  • added UI errors collection for unknown errors

Important files

Discussion points

Next PR will also add validation state checking for duplicates.
Separate PR in the enterprise codebase soon.

@vercel
Copy link

vercel bot commented Aug 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 23, 2023 8:54am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Aug 23, 2023 8:54am

@@ -108,6 +108,10 @@ const useAPI = ({
const response = await res.json();
if (response?.details?.length > 0 && propagateErrors) {
const error = response.details[0];
setErrors(prev => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was missign before. When unknown error was triggered the errors Record was empty. Code that depended on errors being populated (last import stage) failed because of this check missing.

@@ -29,6 +29,11 @@ export const useImportApi = () => {
});
return res;
} catch (e) {
trackEvent('export_import', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added extra tracking for failed imports so that we know when it happens

@@ -289,8 +290,16 @@ export default class FeatureToggleStore implements IFeatureToggleStore {
return this.rowToFeature(row[0]);
} catch (err) {
this.logger.error('Could not insert feature, error: ', err);
if (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we create feature and SQL fails we should not fail silently and return undefined. We want to get instant feedback from code that something was wrong where the failure happened

typeof err.detail === 'string' &&
err.detail.includes('already exists')
) {
throw new NameExistsError(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

special case for duplicate errors

@@ -348,10 +348,21 @@ export default class ExportImportService {
);
}

private async createToggles(dto: ImportTogglesSchema, user: User) {
private async createOrUpdateToggles(dto: ImportTogglesSchema, user: User) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we had a bug in the previous import that you couldn't update toggles themselves. So e.g. if you changes toggle type in import it was ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we detect if feature exists and update it if needed. I wanted to put this code into the feature toggle service itself bu we need createOrUpdate within a project not a global createOrUpdate in any project so for now I decided to leave createOrUpdate in the import subdomain.

@@ -10,7 +10,9 @@ export const isValidField = (
if (!matchingExistingField) {
return true;
}
return importedField.legalValues.every((value) =>
matchingExistingField.legalValues.find((v) => v.value === value.value),
return (importedField.legalValues || []).every((value) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing typechecker errors with legalValues being undefined

@@ -2,11 +2,11 @@ import { Store } from './store';

export interface IContextFieldDto {
name: string;
description?: string;
description?: string | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing typechecked errors

@kwasniew kwasniew requested a review from sjaanus August 23, 2023 08:58
Copy link
Contributor

@sjaanus sjaanus left a comment

Choose a reason for hiding this comment

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

LG

@kwasniew kwasniew merged commit 65e62c6 into main Aug 23, 2023
17 checks passed
@kwasniew kwasniew deleted the fix-import-duplicate-features branch August 23, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants