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 Tasks/Notes created with null position #9068

Merged
merged 4 commits into from
Dec 16, 2024
Merged

Fix Tasks/Notes created with null position #9068

merged 4 commits into from
Dec 16, 2024

Conversation

Weiko
Copy link
Member

@Weiko Weiko commented Dec 13, 2024

Fixes #8810
Fixes #5268
Fixes #8971

  • Fixing Task/Note creation not sending position during creation
  • Adding a command to backfill position being null, using existing backfill command.
  • Removed unused backfill job.
  • Updated workspace entities to set position non-nullable and set a default value to make it non-required on the API
  • Updated position factory to set a default position for all objects having a POSITION field instead of only company/people
  • Moved the try/catch in each resolver factory calling GraphqlQueryRunnerException handler, makes more sense to call it in the actual graphql-query-runner and removing some duplicate codes
  • Adding validations for input in QueryRunnerArgs factories
  • Allow sync-metadata to override and sync defaultValues for certain field types (that can't be updated by users)
  • Removing health-check from sync-metadata command during force mode to improve performances

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

This PR implements comprehensive changes to fix null position handling in Tasks and Notes across the application. Here's a summary of the key changes:

  • Added new 0.40 upgrade command with record position backfill functionality and forced metadata sync
  • Made position fields non-nullable with default value 0 across all workspace entities (Tasks, Notes, Companies, etc.)
  • Centralized error handling by moving try/catch blocks from resolver factories to graphql-query-runner level
  • Removed unused record position backfill job in favor of direct command execution
  • Improved sync-metadata performance by skipping health checks when using force mode

The changes look well-structured and address the core issues while improving overall code organization and performance.

53 file(s) reviewed, 15 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Member

@magrinj magrinj left a comment

Choose a reason for hiding this comment

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

Some comments, but globally good to me


const graphqlQuerySelectedFieldsResult =
graphqlQueryParser.parseSelectedFields(
const resultWithGetters = await this.queryResultGettersFactory.create(
Copy link
Member

Choose a reason for hiding this comment

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

I've take a look inside QueryRunnerArgsFactory, create function return an any type, it seems a bit old, but we can definitely properly type this with function override or union, any shouldn't used

@Weiko Weiko merged commit 5a27491 into main Dec 16, 2024
19 checks passed
@Weiko Weiko deleted the c--fix-8810 branch December 16, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants