twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
GNU Affero General Public License v3.0
15.6k stars 1.65k forks source link

Support custom object renaming #5491

Open ijreilly opened 3 months ago

ijreilly commented 3 months ago

Scope & Context

Currently, renaming a custom object will cause error because table names and foreign key columns names migrations are required and have not been implemented yet in the update object endpoint.

greptile-apps[bot] commented 3 months ago

Disclaimer: This comment was automatically generated with AI. It can give some useful hints as to where to look for, but sometimes it can also give very inaccurate answers so takes this with a pinch of salt :).

To support custom object renaming, follow these steps:

  1. Locate Update Object Endpoint: Modify the update object endpoint in src/controllers/objectController.js.

  2. Implement Table Name Migration: Use the renameTable method in src/services/databaseService.js to rename the table associated with the custom object.

  3. Update Foreign Key Columns: Use the renameColumn method in src/services/databaseService.js to rename foreign key columns referencing the custom object.

  4. Example Code:

    
    // src/controllers/objectController.js
    const { renameTable, renameColumn } = require('../services/databaseService');

async function updateObject(req, res) { const { oldName, newName } = req.body; await renameTable(oldName, newName); await renameColumn('foreign_table', ${oldName}_id, ${newName}_id); // other update logic }



Ensure to handle errors and edge cases appropriately.

### References
FelixMalfait commented 1 month ago

Issue needs to be refined / we said label and api names weren't necessarily correlated. All objects labels could be renamed. I think we introduce an advanced mode to help with that distinction - not sure how we "decorrelate the two" (@Bonapara knows / can decide whether to close and point to other issues)

Bonapara commented 1 month ago

Related to https://github.com/twentyhq/twenty/issues/6145

Faisal-imtiyaz123 commented 1 month ago

@Bonapara Do you want me to work on this as part of 6145? I would love to.

greptile-apps[bot] commented 1 month ago

To support custom object renaming, follow these steps:

  1. Update updateOneObject method in object-metadata.service.ts:

    • Add logic to handle table name and foreign key column name migrations.

      public async updateOneObject(
      input: UpdateOneObjectInput,
      workspaceId: string,
      ): Promise<ObjectMetadataEntity> {
      validateObjectMetadataInputOrThrow(input.update);
      
      const updatedObject = await super.updateOne(input.id, input.update);
      
      if (input.update.nameSingular || input.update.namePlural) {
      await this.handleObjectRenaming(input.id, input.update, workspaceId);
      }
      
      await this.workspaceCacheVersionService.incrementVersion(workspaceId);
      
      return updatedObject;
      }
  2. Implement handleObjectRenaming method in object-metadata.service.ts:

    • Create migrations for renaming table and foreign key columns.

      private async handleObjectRenaming(
      objectId: string,
      update: UpdateObjectPayload,
      workspaceId: string,
      ) {
      const objectMetadata = await this.findOneOrFailWithinWorkspace(workspaceId, { where: { id: objectId } });
      
      const migrations = buildMigrationsForCustomObjectRenaming(objectMetadata, update);
      
      await this.workspaceMigrationService.createCustomMigration(
      generateMigrationName(`rename-${objectMetadata.nameSingular}`),
      workspaceId,
      migrations,
      );
      
      await this.workspaceMigrationRunnerService.executeMigrationFromPendingMigrations(workspaceId);
      }
  3. Create buildMigrationsForCustomObjectRenaming utility function:

    • Define the necessary migrations for renaming tables and foreign key columns.

      export const buildMigrationsForCustomObjectRenaming = (
      objectMetadata: ObjectMetadataEntity,
      update: UpdateObjectPayload,
      ): WorkspaceMigrationTableAction[] => {
      const migrations: WorkspaceMigrationTableAction[] = [];
      
      if (update.nameSingular) {
      migrations.push({
       name: computeObjectTargetTable(objectMetadata),
       action: WorkspaceMigrationTableActionType.RENAME,
       newName: computeObjectTargetTable({ ...objectMetadata, nameSingular: update.nameSingular }),
      });
      }
      
      // Add logic for renaming foreign key columns
      
      return migrations;
      };
  4. Update checkIfFieldIsEditable method in before-update-one-object.hook.ts:

    • Allow renaming of custom objects.

      private checkIfFieldIsEditable(
      update: UpdateObjectPayload,
      objectMetadata: ObjectMetadataEntity,
      ) {
      if (!objectMetadata.isCustom) {
      // Existing checks for non-custom objects
      }
      
      // Allow renaming for custom objects
      }

References

/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts /packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts /packages/twenty-server/src/engine/metadata-modules/object-metadata/utils/build-workspace-migrations-for-custom-object.util.ts /packages/twenty-server/src/engine/metadata-modules/object-metadata/utils/create-migrations-for-custom-object-relations.util.ts /packages/twenty-server/src/engine/metadata-modules/object-metadata/utils/create-workspace-migrations-for-remote-object-relations.util.ts /packages/twenty-server/src/engine/metadata-modules/relation-metadata/relation-metadata.service.ts /packages/twenty-server/src/engine/metadata-modules/object-metadata/utils/build-migrations-for-custom-object-relations.util.ts /packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/remote-table.service.ts /packages/twenty-server/src/engine/metadata-modules/remote-server/remote-table/foreign-table/foreign-table.service.ts /packages/twenty-server/src/engine/workspace-manager/workspace-migration-builder/factories/workspace-migration-relation.factory.ts

#### About Greptile This response provides a starting point for your research, not a precise solution. Help us improve! Please leave a ๐Ÿ‘ if this is helpful and ๐Ÿ‘Ž if it is irrelevant. [Ask Greptile](https://app.greptile.com/chat/github/twentyhq/twenty/main) ยท [Edit Issue Bot Settings](https://app.greptile.com/apps/github)
ijreilly commented 1 month ago

@Bonapara I think we need to clarify if we want to migrate the table names or not upon renaming the objects. (For now that is required for the application to work correctly, beyond the desire of consistency.)

@Faisal-imtiyaz123 thanks for your interest, lets wait until this is clarified !

Bonapara commented 1 month ago

image

Everything should be deducted from the plural label at creation. The singular label should automatically update when the plural label is updated, even if it was overwritten in advanced settings.

The API names should not be updated when the labels are updated.

@FelixMalfait Does this look good to you too?

ijreilly commented 1 month ago

@Bonapara the "api name" needs to follow a series of constraints: being camelCased, only contain latin characters etc .(from the product we do the transliteration from the label). I think some hints may be useful here, maybe a front-end validation with hints as we do here: https://github.com/twentyhq/twenty/issues/6094

Also just my opinion but I am not sure whether this section should show directly for all users, or be semi-hidden behind an expandable section. Thinking of cloud non-tech users here

Bonapara commented 1 month ago

23af7638932672bcd6b8b976daaecdcc46a8af7c00a75b64611a389b82b07c1d

@Bonapara the "api name" needs to follow a series of constraints: being camelCased, only contain latin characters etc .(from the product we do the transliteration from the label). I think some hints may be useful here, maybe a front-end validation with hints as we do here: #6094

@ijreilly just added an info icon at the end of the input. We could also use a helper under the field, but it was redundant.

Also just my opinion but I am not sure whether this section should show directly for all users, or be semi-hidden behind an expandable section. Thinking of cloud non-tech users here

I think it should be hidden. It's under the Advanced Mode settings.

charlesBochet commented 1 month ago

I am not 100% convinced by this feature : 1) if the label change is what is displayed in regular mode but does not change the apiName (nameSingular and namePlural in database), for most of the users the label will diverge from the database model which is an issue for readability using tools such as metabase. I feel that by default both should go together and we would need to introduce a way to in advance mode to: override the labels OR to use the apiNames.

2) For labels we have singular and plural, for names we should have the same

Bonapara commented 1 month ago

For 2. @charlesBochet did you see the screenshot in the thread?

(The idea would be to deduct the singular name for both the API and Labels from the plural label input)

FelixMalfait commented 1 month ago

@charlesBochet I also didn't feel 100% good about the current state. Maybe one option to avoid introducing the overwrite boolean is to build a function that detects automatically if it's been 'unsychronized' for each field? From a UX perspective we could also display the predicted value as "suggested: xxx" if the field has been unsynchronized @Bonapara, that way if a user wants to "re-sync" they can set the suggested value. Even if the fields are not displayed in non-advanced mode, they would still be sent.

It puts the weight on the frontend which isn't great, but I don't think it's that bad, if someone uses the metadata api to update fields, they are tech-saavy enough to be on their own.

Edit : Just an example to be clear:

charlesBochet commented 1 month ago

@FelixMalfait, I still worry about this unsynchronization not being shown in non-advanced mode. I feel the source should actually be the apiName (aka nameSingular + namePlural) and the overwrite should be the label (aka labelSingular + labelPlural).

Regarding your suggestion, I like the the suggested placeholder if unsynced and if they put the same value the FE considers it synced again. Let's see what @Bonapara thinks

FelixMalfait commented 1 month ago

@charlesBochet In non-advanced mode we could show a small warning that indicates it's not going to be synced or something like that (@Bonapara what do you think?)

I disagree with apiName being the source of truth. Label works well for me. You can go from Label -> API Name but can't go from API Name to label (e.g. CamelCase "HelloC" and "Hello C" give the same output) . Also from a user-perspective, in non-advanced mode, they should care about label only.

Bonapara commented 1 month ago

We could introduce the following system:

1) An Advanced Settings option on the Developer Settings page that allows to sync or unsync all labels and API names:

CleanShot 2024-07-15 at 10 53 25

2) Behavior on an object detail page when this sync setting is activated:

CleanShot 2024-07-15 at 10 28 13

3) Behavior on an object detail page when this sync setting is deactivated:

4) Behavior on the Developer advanced Setting activation and deactivation:

5) Object creation

When creating a new object, even if the Sync labels & API names setting is deactivated, the API names should match the labels.

FelixMalfait commented 1 month ago

Good for me @Bonapara! ๐Ÿ‘Œ

gitstart-app[bot] commented 4 weeks ago

Here is the GitStart Ticket for this issue: https://clients.gitstart.com/twenty/5449/tickets/TWNTY-5491

ijreilly commented 13 hours ago

Blocked by https://github.com/twentyhq/twenty/issues/6147 @gitstart-twenty FYI !