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

belindas-closet-nestjs_sprint8_remove-unrelated-fields #134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alasali1
Copy link
Contributor

@alasali1 alasali1 commented Jun 7, 2024

attempt to remove unrelated field

For this I tried to validate all of the fields so that you can only include the fields that apply to the productType.
I am still able to post an object with unrelated fields on postman though. So this is not fixed though this is my attempt.
Would appreciate some feedback.

@alasali1 alasali1 requested a review from a team as a code owner June 7, 2024 05:32
@alasali1 alasali1 requested review from bcko, cshimm, Exochos, Seiyaroo, tinpham5614, brinkbrink and taylorpapke and removed request for a team June 7, 2024 05:32
Copy link
Contributor

@taylorpapke taylorpapke left a comment

Choose a reason for hiding this comment

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

I think this is correct because when I take away the productSizeShoe field, it saves as an empty array:
Screenshot 2024-06-11 at 11 29 30 PM
Screenshot 2024-06-11 at 11 29 15 PM

It's a little difficult to test further since the UI add product functionality is not currently working. I believe the goal was to be able to add a product and not get default numerical values written to the database for productSize's that don't apply to certain productType's.

Correct me if I'm wrong @keiffer213 but this is for unrelated product fields that aren't displayed in the UI and are still setting default values? For example, you can have productType as Shoes, not enter a productSize but it will still show up as size 'S' in the database.

Copy link
Contributor

@IsaacJrTypes IsaacJrTypes left a comment

Choose a reason for hiding this comment

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

Check the following suggestions, I think it will fix some of the issues you are encountering. One behavior I noticed is @ValidateIf will run first (decorators are checked sequentially from top to bottom) and if it fails, the rest of the decorators will not run and validate.

Your code should work as intended once you make the changes, Great Job!

@@ -12,18 +12,22 @@ export class CreateProductDto {
@IsEnum(['MALE','FEMALE', 'NON_BINARY'])
readonly productGender: [];

@ValidateIf((object, value) => object.productType === 'Shoes')
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove value for each time not in use in @ValidateIf decorator

@@ -12,18 +12,22 @@ export class CreateProductDto {
@IsEnum(['MALE','FEMALE', 'NON_BINARY'])
readonly productGender: [];

@ValidateIf((object, value) => object.productType === 'Shoes')
@IsOptional()
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove @IsOptional decorator

line 15 validates if object.productType matches with 'Shoes'. If it does match, then the next decorator is checked. @IsOptional() will create an entry in the database with an empty array if object.productSizeShoe is empty or is not present in the object payload. Removing @IsOptional decorator will allow @isEnum decorator to run so value of productSizeShoe's value is one of the enum values.

Example

  • The image below shows a post request with no productShoeSize in payload.
    CleanShot 2024-07-08 at 15 50 01
  • Removing @IsOptional decorator will return the error below. Doing so will allow the @isEnum decorator to run and validate the payload.
    CleanShot 2024-07-08 at 15 56 07

@bcko
Copy link
Contributor

bcko commented Jul 30, 2024

@alasali1 please address the issue requested by Isaac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants