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(schema) be a little more permissive with mutually exclusive fields #91

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Oct 29, 2019

As part of https://github.com/zapier/zapier/pull/32518, I noticed that the frontend could get into a bad state where it was sending {dict: false, list: false} and schema was complaining that those were mutually exclusive.

We can be a little more permissive here- it's only bad if they're truthy. My one unknown is that I'm not sure how the editor reads these values. If it also uses _.has, then this will cause weird behavior. @stevelikesmusic that's what I wanted to loop you in on if you could double check.

@xavdid xavdid requested a review from eliangcs as a code owner October 29, 2019 01:40
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

Didn't pull but it looks good!

Copy link
Contributor

@stevelikesmusic stevelikesmusic left a comment

Choose a reason for hiding this comment

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

Nice @xavdid! What was the funky state Godzilla was getting into?

I think we should be ok. I don't believe we use _.has to check for those fields.

In Godzilla we do enforce mutual exclusivity by disabling options in the UI for regular input fields. The same is not true for line items where you can have children and dict: true right now.

But I think this should be safe allowing dict: false, list: false

@xavdid
Copy link
Contributor Author

xavdid commented Oct 29, 2019

@stevelikesmusic the was that if you check "list" and then "uncheck it", the state goes {} -> {list: true} -> {list: false} (which makes sense). But, the way that this worked, it was throwing an error.

To be clear, that's only using code from the above linked PR, where we validate VB apps in lambda. so no issue now, but I wanted to get ahead of this one.

@xavdid xavdid merged commit b94c59d into master Oct 29, 2019
@xavdid xavdid deleted the mutually-exclusive-improvements branch October 29, 2019 17:20
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.

3 participants