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 forceId to reject empty strings #1576

Closed
wants to merge 2 commits into from
Closed

Conversation

Yaty
Copy link

@Yaty Yaty commented Apr 28, 2018

Status : WIP

Description

Add allowBlank and allowNull opions to validatesAbsenceOf
Make allowBlank and allowNull to false when forceId if set to true

My test are passing but I broke other tests like this one : https://github.com/strongloop/loopback-datasource-juggler/blob/1ef38a6929ae100c73030b720097ba5c4b90a7b3/test/optional-validation.test.js#L117
It seems that the validation is made before the ID is generated so it breaks.
This is the first time I'm working on this codebase, any tips where I should look :) ?

Related issues

#1519

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@slnode
Copy link

slnode commented Apr 28, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@Yaty Yaty requested review from virkt25 and zbarbuto as code owners April 28, 2018 17:21
@jannyHou
Copy link
Contributor

@slnode test please

@nitro404
Copy link
Contributor

It would probably be beneficial to squash or amend these commits together as they both seem to have the same message, and is a bit confusing, unless there is some reason to keep them separate.

@bajtos
Copy link
Member

bajtos commented May 29, 2018

Hello @Yaty, thank you for the pull request. The proposed changes look reasonable to me, we need to polish the edge cases now.

My test are passing but I broke other tests like this one

I think this is a sign that your change will probably break existing applications. IIUC, the intention is to reject null and empty strings. However, we must still allow undefined as a valid (and required!) value.

Perhaps nullCheck treats undefined the same way as null?

@bajtos bajtos self-assigned this May 29, 2018
@bajtos bajtos changed the title Fix #1519 Fix forceId to reject empty strings May 31, 2018
@bajtos
Copy link
Member

bajtos commented Jan 25, 2019

Closing as abandoned. Feel free to reopen if you get time to work on it again.

@bajtos bajtos closed this Jan 25, 2019
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.

5 participants