Skip to content

Conversation

georgyo
Copy link

@georgyo georgyo commented Nov 1, 2016

There are two problems that are currently affecting the ability to
fetch all results.

  1. In older versions of this code when maxResults was falsy, it would
    then change it to be 50, and then iterate over the code. This was
    changed to a much more ideal method, however the old code blindly put
    the value of maxResults into the dict of search params. It didn't matter
    as it get reassigned pretty fast. The new code still put's it blindly
    into the map, but then never reassigns it, and continues to do a search
    with it to determin how many results there are. Which creates an invalid
    search query. Solution: Remove it from being added to the map. It
    actually never intentionally used in that map without having it
    reassigned first.

  2. There is this isLast code, which likely could be removed entirely.
    It, according to the comment only works with the Jira agile module. If
    jira doesn't know what isLast is, the code would return that is_last is
    True. Obviously this is a bad failure case, as if we don't have the
    agile module we can only ever return one page. Solution: Change the
    default to False.

@review-ninja
Copy link

ReviewNinja

@georgyo
Copy link
Author

georgyo commented Nov 1, 2016

This fixes #229, #253, and #269 by actually fixing the problems and not breaking compatibility with older releases. The other solutions to this problem didn't even seem to look at the code.

@olivierlefloch Pinging you, since you have tried to fix these issues before.

There are two problems that are currently affecting the ability to
fetch all results.

1. In older versions of this code when maxResults was falsy, it would
then change it to be 50, and then iterate over the code. This was
changed to a much more ideal method, however the old code blindly put
the value of maxResults into the dict of search params. It didn't matter
as it get reassigned pretty fast. The new code still put's it blindly
 into the map, but then never reassigns it, and continues to do a search
with it to determin how many results there are. Which creates an invalid
search query. Solution: Remove it from being added to the map. It
actually never intentionally used in that map without having it
reassigned first.

2. There is this isLast code, which likely could be removed entirely.
It, according to the comment only works with the Jira agile module. If
jira doesn't know what isLast is, the code would return that is_last is
True. Obviously this is a bad failure case, as if we don't have the
agile module we can only ever return one page. Solution: Change the
default to False.
@georgyo georgyo force-pushed the fix_fetching_all_records branch from 5383faf to 47faf39 Compare November 1, 2016 19:42
@atulatri
Copy link

atulatri commented Nov 4, 2016

+1

@ssbarnea ssbarnea force-pushed the develop branch 8 times, most recently from 47fd467 to 743081e Compare November 11, 2016 16:25
@ssbarnea
Copy link
Member

Closing because this seems to make maxResults parameter not functional. Still, is this in not yet solved I would recommend writing a small unittest for testing this bug.

@ssbarnea ssbarnea closed this Nov 22, 2016
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.

4 participants