Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(filter): filter on false properties #3597

Closed
wants to merge 1 commit into from

Conversation

tomdcc
Copy link
Contributor

@tomdcc tomdcc commented Aug 15, 2013

Code was evaluating !expression[key] while attempting to
see if the key was present, but this was evaluating to true for
false values as well as missing keys.

Closes #2797.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@tomdcc
Copy link
Contributor Author

tomdcc commented Aug 15, 2013

Real name: Thomas Dunstan

@@ -183,7 +183,7 @@ function filterFilter() {
})();
} else {
(function() {
if (!expression[key]) return;
if (typeof(expression[key]) === undefined) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

typeof returns a string so this comparison is never true. Use typeof expression[key] === 'undefined'. You should also include one item where the property is not defined in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Also, how weird that it wasn't picked up by any other tests. I'll remedy that, too.

Code was evaluating !expression[key] while attempting to
see if the key was present, but this was evaluating to true for
false values as well as missing keys.

Closes angular#2797.
@ghost ghost assigned vojtajina Aug 15, 2013
@vojtajina
Copy link
Contributor

Merged as 3bc4e7f. Thanks @tomdcc !

@vojtajina vojtajina closed this Aug 15, 2013
@tomdcc
Copy link
Contributor Author

tomdcc commented Aug 15, 2013

Thanks for merging! Now I can upgrade off of 1.0.x.

@tomdcc tomdcc deleted the filter-on-false-property branch August 15, 2013 23:14
royling added a commit to royling/angular.js that referenced this pull request Jan 4, 2014
use only one IIFE and a ternary op in it, instead of invoking separate IIFEs in if-else (this also completely fixed the same issue closed by PR angular#3597)
also add a spec to verify usage of '$' property in expression object (e.g. `{$: 'a'}`)
royling added a commit to royling/angular.js that referenced this pull request Jan 5, 2014
- use only one IIFE and a ternary op in it, instead of invoking separate IIFEs in if-else
(this also completely fixed the same issue closed by PR angular#3597)
- also add a spec to verify usage of '$' property in expression object (e.g. `{$: 'a'}`)
royling added a commit to royling/angular.js that referenced this pull request Jan 5, 2014
- use only one IIFE and a ternary op in it, instead of invoking separate IIFEs in if-else
(this also completely fixed the same issue closed by PR angular#3597)
- also add a spec to verify usage of '$' property in expression object (e.g. `{$: 'a'}`)
IgorMinar pushed a commit that referenced this pull request Jan 5, 2014
- use only one IIFE and a ternary op in it, instead of invoking separate IIFEs in if-else
(this also completely fixed the same issue closed by PR #3597)
- also add a spec to verify usage of '$' property in expression object (e.g. `{$: 'a'}`)

Closes #5637
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
- use only one IIFE and a ternary op in it, instead of invoking separate IIFEs in if-else
(this also completely fixed the same issue closed by PR angular#3597)
- also add a spec to verify usage of '$' property in expression object (e.g. `{$: 'a'}`)

Closes angular#5637
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
- use only one IIFE and a ternary op in it, instead of invoking separate IIFEs in if-else
(this also completely fixed the same issue closed by PR angular#3597)
- also add a spec to verify usage of '$' property in expression object (e.g. `{$: 'a'}`)

Closes angular#5637
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angularjs 1.1.5: ngRepeat not filtering correctly[UPDATE]
4 participants