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

Deprecate Probability field on Opportunity #6207

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Conversation

ijreilly
Copy link
Collaborator

Closes #5735.
The field probability on opportunity will -

  • stop being created for new workspaces (after this PR is merged)
  • have "isCustom" value set to true and be displayed as such in the settings (after this PR is merged + sync-metadata is run on workspace)
  • still show in the views (all the time)

This field is deprecated as a standard field but not replaced by another one, so we are not adding the (deprecated) suffix in the label.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • Removed 'probability' field from GraphQL queries in packages/twenty-front/src/modules/object-record/hooks/__mocks__/useFindManyRecords.ts and packages/twenty-front/src/modules/search/hooks/__mocks__/useFilteredSearchEntityQuery.ts
  • Removed 'probability' field from mock data in packages/twenty-front/src/modules/sign-in-background-mock/constants/SignInBackgroundMockCompanies.ts and packages/twenty-front/src/testing/mock-data/view-fields.ts
  • Removed 'probability' field from seed data in packages/twenty-server/src/database/typeorm-seeds/workspace/opportunities.ts
  • Removed 'probability' field from data generation in packages/twenty-server/src/engine/workspace-manager/demo-objects-prefill-data/opportunity.ts
  • Deprecated 'probability' field in packages/twenty-server/src/modules/opportunity/standard-objects/opportunity.workspace-entity.ts

9 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@@ -251,7 +251,7 @@ export const OPPORTUNITY_STANDARD_FIELD_IDS = {
name: '20202020-8609-4f65-a2d9-44009eb422b5',
amount: '20202020-583e-4642-8533-db761d5fa82f',
closeDate: '20202020-527e-44d6-b1ac-c4158d307b97',
probability: '20202020-69d4-45f3-9703-690b09fafcf0',
probability_deprecated: '20202020-69d4-45f3-9703-690b09fafcf0',
Copy link
Member

Choose a reason for hiding this comment

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

Should we use probabilityDeprecated instead to remain consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops sure, thanks

Copy link
Member

Choose a reason for hiding this comment

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

I saw Address_old was this way also (and did a quick PR fix for database:reset, make sure this PR doesn't break it also), maybe still time to fix it for Address

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops yes still time thanks for catching this

type: FieldMetadataType.TEXT,
label: 'Probability',
description: 'Opportunity probability',
icon: 'IconProgressCheck',
defaultValue: "'0'",
})
@WorkspaceIsDeprecated()
Copy link
Member

Choose a reason for hiding this comment

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

Could we move all deprecated fields at the bottom of files by convention?

type: FieldMetadataType.TEXT,
label: 'Probability',
description: 'Opportunity probability',
icon: 'IconProgressCheck',
defaultValue: "'0'",
})
@WorkspaceIsDeprecated()
probability: string;
Copy link
Member

Choose a reason for hiding this comment

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

should this also be renamed to probabilityDeprecated? (if we want to introduce a new probability field soon?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im not sure - I don't think we have planned to introduce a new probability field for now, so the vision is to consider this field custom, not to discourage its usage for now. its what I had shared in the doc (doc is still to be completed though and migrated to github). but open to rediscuss this

Copy link
Member

Choose a reason for hiding this comment

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

Ok!

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Renamed probability_deprecated to probabilityDeprecated in packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/constants/standard-field-ids.ts
  • Re-added deprecated address_old field in packages/twenty-server/src/modules/company/standard-objects/company.workspace-entity.ts
  • Deprecated probability field in packages/twenty-server/src/modules/opportunity/standard-objects/opportunity.workspace-entity.ts using @WorkspaceIsDeprecated() decorator

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@ijreilly ijreilly merged commit 5ebde33 into main Jul 11, 2024
13 of 14 checks passed
@ijreilly ijreilly deleted the deprecate-probability-field branch July 11, 2024 12:50
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Removed Timeline.tsx component, indicating potential deprecation or relocation of timeline functionality (packages/twenty-front/src/modules/activities/timeline/components/Timeline.tsx)
  • Added shouldMatchRootQueryFilter parameter to control cache updates in triggerCreateRecordsOptimisticEffect, useCreateManyRecords, and useCreateOneRecord hooks (packages/twenty-front/src/modules/apollo/optimistic-effect/utils/triggerCreateRecordsOptimisticEffect.ts, packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts, packages/twenty-front/src/modules/object-record/hooks/useCreateOneRecord.ts)
  • Updated CSS height property from 100vh to 100dvh for better viewport handling across multiple components (packages/twenty-front/src/loading/components/LeftPanelSkeletonLoader.tsx, packages/twenty-front/src/loading/components/UserOrMetadataLoader.tsx, etc.)
  • Introduced new command to add a new address field to views with deprecated address fields (packages/twenty-server/src/database/commands/0-22-add-new-address-field-to-views-with-deprecated-address.command.ts)
  • Simplified TimelineCreateButtonGroup by removing openCreateActivity function and targetableObject prop (packages/twenty-front/src/modules/activities/timeline/components/TimelineCreateButtonGroup.tsx)

28 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

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.

[Timebox] Remove "Probability" Field from Opportunities
3 participants