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

Data explorer #6159

Closed
wants to merge 71 commits into from
Closed

Data explorer #6159

wants to merge 71 commits into from

Conversation

ad-elias
Copy link
Contributor

@ad-elias ad-elias commented Jul 8, 2024

Building report and chart creation.

Screenshot 2024-07-08 at 12 41 47

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

  • Added new routes for Reports, Charts, and ChartEditor in /packages/twenty-front/src/App.tsx
  • Introduced AnalyticsQueryEditor, AnalyticsQueryFilter, and AnalyticsQueryFilters components in /packages/twenty-front/src/modules/activities/reports/components/
  • Added Chart, ChartConfig, PageAddReportButton, ReportGroup, ReportGroups, ReportRow, and ReportsLayout components in /packages/twenty-front/src/modules/activities/reports/components/
  • Defined new types and constants for reports and charts in /packages/twenty-front/src/modules/activities/reports/types/ and /packages/twenty-front/src/modules/activities/reports/constants/ReportGroupTimeSpans.ts
  • Introduced IS_REPORTS_ENABLED feature flag and updated related files in /packages/twenty-server/src/engine/core-modules/feature-flag/feature-flag.entity.ts and /packages/twenty-front/src/modules/workspace/types/FeatureFlagKey.ts

30 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings

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 a console.log statement used for debugging in /packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/services/workspace-sync-relation-metadata.service.ts

No major changes found since last review.

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

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Great work. Good that you opened the PR because I think there might be some wasted work, but it's the right direction!

description: 'Singular name of the source object',
icon: 'IconAbc',
})
sourceObjectNameSingular: string;
Copy link
Member

Choose a reason for hiding this comment

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

It's better to point to the objectMetadataId instead, but you don't even need this (cf comment below, fieldMetadataId should be enough to retrieve everything)

description: 'Dot-separated path to the source field.',
icon: 'IconForms',
})
fieldPath: string;
Copy link
Member

Choose a reason for hiding this comment

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

use fieldMetadataId instead and retrieve the truth from the source in metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think something like fieldPath is required to be able to do joins - do we want to do joins? Do you have another idea how to do joins?

objectNameSingular: CoreObjectNameSingular.Chart,
});

return (
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 want to create a page dedicated to Charts ideally, instead we would want to try leveraging the generic /charts view that is generated from Charts being a standard object. Otherwise we're creating more tech debt. It might require a few adjustments (e.g. hijack + button?) but it should be doable

Copy link
Member

Choose a reason for hiding this comment

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

(I know we didn't it for task but we actually plan to migrate tasks whenever we can and we regret creating a dedicated page for it)

packages/twenty-front/src/pages/reports/Reports.tsx Outdated Show resolved Hide resolved
import { WorkspaceJoinColumn } from 'src/engine/twenty-orm/decorators/workspace-join-column.decorator';

@WorkspaceEntity({
standardId: STANDARD_OBJECT_IDS.analyticsQueryFilter,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how much of this is the same as viewFilter and should be 100% shared? let's keep them as close as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this also depends on whether we want to be able to create JOIN queries or not. If not, I think we can use view-filter, otherwise a separate entity is required.

export const Charts = () => {
const reportId = useParams().reportId ?? '';

const navigate = useNavigate();
Copy link
Member

Choose a reason for hiding this comment

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

FYI even if this page goes away, let's try to avoid using useNavigate() when we can and use instead which are better for accessibility and other reasons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should we use instead? import { Link } from 'react-router-dom'; ?

Copy link
Contributor Author

@ad-elias ad-elias Jul 22, 2024

Choose a reason for hiding this comment

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

"The link on the bottom left should be seen on hover"
Also, open in new tab should work!

@FelixMalfait FelixMalfait self-assigned this Jul 8, 2024
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)

  • Added AnalyticsQueryModule to packages/twenty-server/src/engine/core-modules/core-engine.module.ts
  • Introduced AnalyticsQueryService in packages/twenty-server/src/engine/core-modules/analytics-query/analytics-query.service.ts
  • Created AnalyticsQueryResolver in packages/twenty-server/src/engine/core-modules/analytics-query/analytics-query.resolver.ts
  • Added GraphQL mutation RUN_ANALYTICS_QUERY in packages/twenty-front/src/modules/analytics-query/graphql/queries/runAnalyticsQuery.ts
  • Implemented UI components for analytics queries in packages/twenty-front/src/modules/activities/reports/components/

11 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings

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 routes for ChartEditor, Charts, and Reports pages (packages/twenty-front/src/App.tsx)
  • Deleted multiple report-related components (PageAddReportButton.tsx, ReportGroup.tsx, ReportGroups.tsx, ReportRow.tsx, ReportsLayout.tsx, Reports.tsx)
  • Removed 'report' and 'reportId' properties from Chart interface (packages/twenty-front/src/modules/activities/reports/types/Chart.ts)
  • Renamed feature flag from 'IS_REPORTS_ENABLED' to 'IS_CHARTS_ENABLED' (packages/twenty-server/src/engine/core-modules/feature-flag/feature-flag.entity.ts, add-standard-id.command.ts)
  • Removed ReportWorkspaceEntity and related fields (chart.workspace-entity.ts, report.workspace-entity.ts)

22 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

packages/twenty-front/src/pages/reports/Charts.tsx Outdated Show resolved Hide resolved
packages/twenty-front/src/pages/reports/Charts.tsx Outdated Show resolved Hide resolved
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)

  • Moved AnalyticsQueryEditor.tsx, AnalyticsQueryFilter.tsx, AnalyticsQueryFilters.tsx, Chart.tsx, ChartConfig.tsx, ReportGroupTimeSpans.ts, AnalyticsQuery.ts, AnalyticsQueryFilter.ts, and Chart.ts from reports to charts directory (packages/twenty-front/src/modules/activities/charts/components/)
  • Deleted useIsReportsPage.ts (packages/twenty-front/src/modules/navigation/hooks/)
  • Updated import paths in analytics-query.service.ts (packages/twenty-server/src/engine/core-modules/analytics-query/analytics-query.service.ts)
  • Updated import paths in index.ts (packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/standard-objects/index.ts)
  • Moved analytics-query-filter.workspace-entity.ts, analytics-query.workspace-entity.ts, and chart.workspace-entity.ts from reports to charts module (packages/twenty-server/src/modules/charts/standard-objects/)

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

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)

  • Updated import paths in packages/twenty-front/src/modules/activities/charts/components/AnalyticsQueryEditor.tsx
  • Added state management and mutation for analytics queries in packages/twenty-front/src/modules/activities/charts/components/AnalyticsQueryEditor.tsx
  • Renamed AnalyticsQueryFilterWorkspaceEntity to ChartFilterWorkspaceEntity in packages/twenty-server/src/modules/charts/standard-objects/chart-filter.workspace-entity.ts
  • Removed AnalyticsQueryWorkspaceEntity and added ChartFilterWorkspaceEntity in packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/standard-objects/index.ts
  • Enhanced ChartWorkspaceEntity with new fields and relationships in packages/twenty-server/src/modules/charts/standard-objects/chart.workspace-entity.ts

13 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings

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)

  • Updated AnalyticsQueryEditor.tsx to use Chart type instead of AnalyticsQuery
  • Renamed AnalyticsQueryFilter.tsx to ChartFilter.tsx and updated props
  • Refactored AnalyticsQueryFilters.tsx to align with new Chart and ChartFilter types
  • Integrated useFindOneRecord hook in Chart.tsx for data fetching and loading state
  • Added support for displaying charts in RecordShowContainer.tsx and ShowPageRightContainer.tsx

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

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 runAnalyticsQuery mutation and button in packages/twenty-front/src/modules/activities/charts/components/AnalyticsQueryEditor.tsx
  • Added button to run chart query in packages/twenty-front/src/modules/activities/charts/components/Chart.tsx
  • Renamed mutation from RunAnalyticsQuery to RunChartQuery in packages/twenty-front/src/modules/analytics-query/graphql/queries/runAnalyticsQuery.ts
  • Deleted ChartEditor.tsx and Charts.tsx in packages/twenty-front/src/pages/reports
  • Updated UUIDs in CHART_STANDARD_FIELD_IDS in packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/constants/standard-field-ids.ts

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

JSON.stringify(result, undefined, 2),
);

return { chartResult: JSON.stringify(result) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic: Replace placeholder return value with actual query result processing.

@charlesBochet
Copy link
Member

I'm going to close this PR as it's not ready to merge and we want to keep a clean history but let's keep going on it in September!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants