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

[MINT-3241] Delete categories #1565

Open
wants to merge 5 commits into
base: feature/MINT-3175_mto
Choose a base branch
from

Conversation

OddTomBrooks
Copy link
Contributor

@OddTomBrooks OddTomBrooks commented Dec 11, 2024

MINT-3241

Description

Added server endpoint to delete MTO categories, rebinding references to the parent category reference (or NULL) and recursively deleting subcategories

How to test this change

  1. Run unit tests and validate results
  2. Run Postman MTO Category Delete query and validate behavior against the database

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.
  • Updated the Postman Collection if necessary.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@OddTomBrooks OddTomBrooks changed the base branch from main to feature/MINT-3175_mto December 11, 2024 08:49
migrations/V193__Add_MTO_Category_Delete_Trigger.sql Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This migration name needs to be updated, I think the next available one at the moment is 200

Copy link
Contributor

Choose a reason for hiding this comment

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

@OddTomBrooks , can you update this please? DB migrate is failing

pkg/sqlqueries/SQL/mto/category/delete.sql Show resolved Hide resolved
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

thanks Tom, this is looking good. I left a couple of questions and suggestions. Please let me know if you have any specific questions you want feedback on. Otherwise, I'll plan on doing a full review when you promote the PR.

migrations/V193__Add_MTO_Category_Delete_Trigger.sql Outdated Show resolved Hide resolved
@OddTomBrooks OddTomBrooks marked this pull request as ready for review December 11, 2024 19:46
@OddTomBrooks OddTomBrooks requested review from a team as code owners December 11, 2024 19:46
@OddTomBrooks OddTomBrooks requested review from StevenWadeOddball, patrickseguraoddball and garyjzhao and removed request for a team December 11, 2024 19:46
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

Thanks @OddTomBrooks , this is looking good. The app won't run currently because of the migration numbering. I provided some feedback, I will functionally test this when you have a chance to implement it

END IF;

-- Recursively gather all descendant categories, including OLD.id
WITH RECURSIVE cat_tree AS (
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I'd suggest we don't use recursion. We only plan on allowing at most one level of depth, and recursion adds a greater overhead to the query. (from some past experiences and performance hits, if I can avoid recursion, I do)

SELECT array_agg(id) INTO cat_ids
FROM mto_category
WHERE parent_id = OLD.id OR id = OLD.id;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has not been a simple insertion, I'll revisit it early tomorrow.

pkg/storage/mto_category.go Show resolved Hide resolved
Copy link
Contributor

@StevenWadeOddball StevenWadeOddball left a comment

Choose a reason for hiding this comment

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

it looks like the unit tests for the delete are failing now. Could you take a look please and re-request review when it's corrected?

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.

2 participants