Cadasta / cadasta-qgis-plugin

[DEPRECATED] QGIS plugin to create, edit, upload and download Cadasta projects
GNU General Public License v3.0
4 stars 7 forks source link

Support paginated responses #154

Closed alukach closed 7 years ago

alukach commented 7 years ago

A PR has been made on the Cadasta Platform codebase to add pagination to all "list" API endpoints: Cadasta/cadasta-platform#1425. Naturally, that change will break the QGIS plugin.

This PR adds the ability for the QGIS plugin to follow pagination links. It works by altering the NetworkMixin to have a new connect_get_paginated() method. The flow is as follows:

  1. Makes a normal GET request to the desired endpoint
  2. Analyzes the response for a next property, which will be a URL to the next page of data
  3. Creates a new NetworkMixin instance and calls the connect_get_paginated() method on that instance, passing along the calling instance's API response as a payload and its_set_results_and_complete() method as a callback.
  4. As each request gets a response from the API, it merges the new response with the response from the last API call.
  5. When a response is returned without a next property, the API response handler calls the_set_results_and_complete() callback with the final combined response object.
  6. _set_results_and_complete() of the original instance sets the response data to the NetworkMixin that first called connect_get_paginated() and sets its internal pagination_exhausted flag to True.

Most of the codebase looks at reply.isFinished() to determine if a API request is complete. Being that the pattern of using reply.isFinished() to signal when the request is complete does not really apply to paginated data, a new is_finished(paginated=False) method was added to the NetworkMixin, returning self.reply.isFinished() if the request is a normal request and self.pagination_exhausted flag if the request is a paginated request.

Additionally, along the way I noticed that many classes unnecessarily overrode their parent class' __init__ or connection_finished classes. I went ahead and removed these methods. Additionally, it seemed to make sense to make request_url a required input to the NetworkMixin class.

I've tested these changes out functionally but did not run unit tests nor did I write any unit tests.

@meomancer @dimasciput This code change may be out of scope for this project. I'd appreciate any comments you can offer or instructions regarding how to run the unit-tests.

alukach commented 7 years ago

Looking at the Travis build, I see that the tests run against demo.cadasta.org. These tests will fail until cadasta/cadasta-platform#1425 is merged to production. We may have to either bend that rule and run tests locally against a dev server or wait until the PR is merged to staging and run against that server.

oliverroick commented 7 years ago

Tests should never be run against demo. If tests need a live server, it should be staging. I'd favor a solution that uses some local instance though.

alukach commented 7 years ago

@oliverroick agreed, but not sure how that impacts this PR. Doing a search we can see that tests are run against demo.cadasta.org in a few spots. It feels like our options are to:

I'm leaning towards mocking out the network calls. This would make these more of a standard unit test rather than integration tests. It's the least amount of work, would allow us to get rid of storing credentials in the repo, could ensure that automated tests work for my branch. We lose the confidence of knowing that the API works in the way that the plugin's codebase expects.

Thoughts? Additionally, should this be done in this PR or another?