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

Make returned payloads consistent across Label and Tag CRUD ops #302

Open
nathanielrindlaub opened this issue Jan 21, 2025 · 1 comment
Open
Assignees
Labels

Comments

@nathanielrindlaub
Copy link
Member

createProjectLabel returns just the newly created label but createProjectTag returns all tags in the project. updateProject[Label/Tag} behaves the same way. deleteProjectLabel returns StandardPayload, but deleteTag returns all project.tags. We should pick one approach and make it uniform. Will require some updates to the handling on the frontend in both projectsSlice.js and filtersSlice.js.

Note: in the case of deleting Tags and Labels, it doesn't technically matter what we return because we immediately clear images and fetch for the fresh project after successful deletions. But that's a frontend-specific implementation detail, so I think it's probably best to return the updated list of project.tags and labels even if we aren't using them at them moment.

Related: #293

@nathanielrindlaub
Copy link
Member Author

nathanielrindlaub commented Jan 22, 2025

I made the create and update payloads the same for Labels and Tags, but I held off on making the deleteProjectTags and deleteProjectLabels payloads consistent for now because it seems like the new implementation of deleteLabel utilizes the isOk part of the payload in a few places and I'm not totally tracking what it's for. It seems like isOk: false only can happen if the bulkWrite(operations) returns a bad response in ImageModel.deleteLabelsFromImages():

    const res = await Image.bulkWrite(operations);
    if (!res.isOk()) {
      return {
        isOk: false,
        isOverLimit: false,
      };
    }

But if we get a bad response from the DB there, I assume it will throw an error and the res.isOk() check would get skipped? If so maybe we don't need the isOk part of the payload and we can make the deletes more uniform. Should probably check with @lessej first.

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

No branches or pull requests

1 participant