Skip to content
This repository has been archived by the owner on Jan 27, 2020. It is now read-only.

fixing bug when id is array of string all #349

Closed
wants to merge 5 commits into from

Conversation

esunder
Copy link
Contributor

@esunder esunder commented Oct 7, 2017

No description provided.

@esunder esunder mentioned this pull request Oct 7, 2017
@codecov
Copy link

codecov bot commented Oct 7, 2017

Codecov Report

Merging #349 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #349   +/-   ##
======================================
  Coverage    8.95%   8.95%           
======================================
  Files         234     234           
  Lines        2568    2568           
======================================
  Hits          230     230           
  Misses       2338    2338
Impacted Files Coverage Δ
src/features/Gw2/actions.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1b6256...da41803. Read the comment docs.

@esunder
Copy link
Contributor Author

esunder commented Oct 7, 2017

Note: This PR does not fix the pre-commit warnings/errors that I raised on #223

@@ -66,7 +66,7 @@ export function generateActions (resourceName, getResource, afterGet) {
// We filter them, then pass them along to the service (for example lib/gw2/readCalculatedItemStats)
// If calculatedId exists, use that.
const actualId = +(typeof id === 'object' ? (id.calculatedId || id.id) : id);
const isValidId = actualId && actualId !== -1;
const isValidId = (id === 'all') || (actualId && actualId !== -1);
Copy link
Owner

@itsdouges itsdouges Oct 8, 2017

Choose a reason for hiding this comment

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

(thanks for your first contribution! :))

is this really needed though? the current logic actualId && actualId !== -1 would return true for the string 'all'.

> 'all' && 'all' !== -1
true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actualId was not 'all', it was NaN. Maybe because of the + right after the assignment equals?

Copy link
Owner

Choose a reason for hiding this comment

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

ah, good catch. that'll do it.

can you add a test case for this pls? the reducer/action logic is pretty well tested (compared to the rest of the code base.. haha)

@esunder
Copy link
Contributor Author

esunder commented Oct 8, 2017

Closing this PR for now. I separated the bugfix out from the achievement feature.

@esunder esunder closed this Oct 8, 2017
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.

3 participants