Unique subscriber counts #562

OwlHute · 2019-04-03T12:33:00Z

Closes #588

I have:

Formatted any Python files with black
Brought the branch up to date with master
Added any relevant Github labels
Added tests for any new additions
Added or updated any relevant documentation
Added an Architectural Decision Record (ADR), if appropriate
Added an MPLv2 License Header if appropriate
Updated the Changelog

Description

Description of what this PR does, and what it changes.

maxalbert · 2019-04-03T12:39:07Z

flowclient/flowclient/client.py

+    Example
+    -------
+    >>> unique_subscriber_counts( { "start_date" : "2019-03-20", "end_date" : "2019-03-31", "aggregation_unit" : "admin3" } )
+    [ ]  # TODO - Fill this in


Accidentally forgot to delete this TODO item?

I wasn't sure what goes there. Is there a systematic way to find out? I assume I can try actually running the code and extracting the relavent string from the output.

Yes, you'd need to actually run it and copy & paste the output.

Note that in order to be able to successfully run it, you will need to change the dates to something between 2016-01-01 and 2016-01-07 because the data in the test database is between those dates.

flowclient/flowclient/client.py

maxalbert · 2019-04-03T12:56:52Z

The additions to the API spec (in the file tests/flowmachine_server_tests/test_server.test_api_spec_of_flowmachine_query_schemas.approved.txt) look correct, but they are in the "wrong" order (the keys in the file are sorted alphabetically so that the result is reproducible for the tests).

I assume you edited this file manually? While this is possible, it's easier to let ApprovalTests auto-generate it for you. When you run the relevant integration test (e.g. via cd integration_tests && pipenv run -svx tests/flowmachine_server_tests/test_server) it will open a diff tool and allow you to automatically accept the changes.

However, currently it is configured to use opendiff which I'm not sure is available on Windows. You will want to install some diff tool (see here for some choices) and temporarily change the setting inintegration_tests/tests/conftest.py(near the bottom) from opendiff to whatever tool you installed. Apologies that this is a bit clunky at the moment - we should look into supporting a variety of tools out of the box (which we can do in a separate PR once we have got it working for you on Windows).

maxalbert · 2019-04-03T15:56:51Z

@OwlHute FYI, in PR #566 I added a bit of documentation about the approvaltests-based tests and added a toplevel approvaltests_diff_reporters.json file which allows to configure the diff tool more easily. See here for the addition to the docs about this.

codecov · 2019-04-04T07:37:03Z

Codecov Report

Merging #562 into master will decrease coverage by <.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
- Coverage   92.74%   92.74%   -0.01%     
==========================================
  Files         115      116       +1     
  Lines        6038     6062      +24     
  Branches      672      673       +1     
==========================================
+ Hits         5600     5622      +22     
- Misses        312      314       +2     
  Partials      126      126

Impacted Files	Coverage Δ
flowauth/backend/flowauth/models.py	`93.29% <ø> (ø)`	⬆️
flowclient/flowclient/client.py	`92.34% <100%> (+0.08%)`	⬆️
...e/server/query_schemas/unique_subscriber_counts.py	`100% <100%> (ø)`
...ine/core/server/query_schemas/flowmachine_query.py	`45.94% <33.33%> (-1.12%)`	⬇️

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 c330e5c...3e50c9b. Read the comment docs.

OwlHute · 2019-04-04T07:49:02Z

Re Max's flowmachine_server_tests comment, for now I have manually moved the unique_subscriber_counts entries into alphabetical order, as I have had trouble running the integration_tests:

Firstly, I have established that these tests definitely won't run in Windows, with or without using the FlowKit integration test pipenv environment, because two 3rd party modules they use, "pglast" and "ujson" are Unix-only. (The only other Unix dependency I've come across in FlowKit is the use of "pwd", for dealing with the Unix password database.)

I am able to run them (without missing module failures) in "Ubuntu for Windows" in their FlowKit development pipenv environment, which is good. However, this fails with hordes of errors, all I think due to the same thing: a database port not being configured, or its config not being picked up by the test scripts. Here is a sample error message:

  conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
E sqlalchemy.exc.OperationalError: (psycopg2.OperationalError) could not connect to server:
Connection refused
E Is the server running on host "localhost" (127.0.0.1) and accepting
E TCP/IP connections on port 9997?

Also, I see that after this commit one or more (?) of the automated CI tests have failed. But I can't see anything relating to Unique Subscriber Counts in the failure summary, or in any of the voluminous waffle following it! I'll copy and paste the full test output schpiel and search for any relevant failure reason(s). But at first sight it looks as if the failures may be unrelated to my changes!

greenape · 2019-04-04T08:29:49Z

@OwlHute There's a readme file that explains how to run the integration tests, located in the integration tests folder. Note that you'll need/want to use Pipenv, because various settings are in .env files that pipenv and docker both pick up automatically. You should be able to run the tests using pipenv install && pipenv run run-tests from the integration tests folder.

The test that is currently failing (which I believe @maxalbert has just fixed for you), is the autogenerated api schema.

Once the tests are all passing, we'll also want to look at the test coverage to assess whether you need to add some more tests to cover the new code.

maxalbert · 2019-04-04T08:34:33Z

Thanks for the changes @OwlHute! The toplevel keys are now in the correct order. The nested keys were still in non-alphabetical order; for the sake of expediency I just pushed a change to fix this (you will want to run git pull to bring your local branch up to date).

Apologies for the test run failures. This is due to #467, I'll submit a PR to fix this in a minute. In the meantime, you should be able to run the tests by setting the following environment variables explicitly in your shell (you will need to run pipenv shell first to activate the pipenv environment, otherwise the integration_tests/.env file overwrites them again).

cd integration_tests
pipenv shell
export FLOWDB_PORT=9000
export REDIS_PORT=6379
export FLOWMACHINE_PORT=5555

Are you able to run the tests with these settings?

If you want to run just the API spec test (either for speed, or to avoid failures with Unix-only modules) you can do it like this (from within the integration tests pipenv environment as above):

pytest -svx tests/flowmachine_server_tests/test_server.py

maxalbert · 2019-04-04T08:39:54Z

@greenape Is there a need for any more test coverage or are all the changes here covered by existing tests?

OwlHute · 2019-04-04T08:59:14Z

@max, many thanks for these port settings. I ran the test in the pipenv environment in Ubuntu, and the output looks a lot better, in that it is much shorter! But there are still a few errors around connections, e.g.:

          sock.connect(socket_address)
E ConnectionRefusedError: [Errno 111] Connection refused

E ConnectionRefusedError: [Errno 111] Connection refused

/home/jr/.local/share/virtualenvs/integration_tests-sEjdA8oo/lib/python3.7/site-packages/redis/connection.py:538: ConnectionRefusedError

:::

/home/jr/.local/share/virtualenvs/integration_tests-sEjdA8oo/lib/python3.7/site-packages/redis/connection.py:497: ConnectionError
-------------------------------------------------- Captured log setup > --------------------------------------------------connection.py 103 INFO {"event":"Couldn't
get username for application name, using >'flowmachine'","logger":"flowmachine.core.connection","level":"info","timestamp":"2019-04-04T08:54:32.554599Z"}
init.py 213 INFO Logger created with level DEBUG

greenape

Thanks.

Needs tests, because this drops the coverage a little too much.

Two approaches there, first is to add a tavern style test, as seen here https://github.com/Flowminder/FlowKit/tree/master/integration_tests/tests/flowapi_tests + a test for constructing the dict in the FlowClient tests, here: https://github.com/Flowminder/FlowKit/tree/master/flowclient/tests/unit

Alternative would be to add a test to the parameterised test_run_query test here: https://github.com/Flowminder/FlowKit/blob/master/integration_tests/tests/test_queries.py which would hit both code paths.

greenape · 2019-04-04T08:47:31Z

flowmachine/flowmachine/core/server/query_schemas/unique_subscriber_counts.py

+    @property
+    def _flowmachine_query_obj(self):
+        """
+        Return the underlying flowmachine daily_location object.


Suggested change

Return the underlying flowmachine daily_location object.

Return the underlying flowmachine UniqueSubscriberCounts object.

I've implemented the suggested alternative, but that _flowmachine_query_ob j() method was already present and the only change I made was to fix the object name in theader. So that won't change the test coverage. Presumably something must be added to actually call _flowmachine_query_obj() ?! (I assumed some test would run it automatically, as its name looks pretty generic.)

@OwlHute This particular suggestion about changing daily_location to UniqueSubscriberCounts in the docstring is independent of @greenape's comment about code coverage and the need to add an additional test. (It may look related because Github displays it right underneath, but it has nothing to do with it).

greenape · 2019-04-04T08:49:21Z

flowclient/flowclient/client.py

+    Example
+    -------
+    >>> unique_subscriber_counts( { "start_date" : "2019-03-20", "end_date" : "2019-03-31", "aggregation_unit" : "admin3" } )
+    [ ]  # TODO - Fill this in


Example needs to be filled in as mentioned, and should actually work when run (args are wrapped in a dict here, so this would error when run?)

greenape · 2019-04-04T09:13:37Z

For reference, I'm looking at the coverage report for the added schema here: https://codecov.io/gh/Flowminder/FlowKit/src/73abcbf0213132937f6cb1d3b11640bf4e0a5b6e/flowmachine/flowmachine/core/server/query_schemas/unique_subscriber_counts.py

and the client, here: https://codecov.io/gh/Flowminder/FlowKit/src/unique_subscriber_counts/flowclient/flowclient/client.py

…eader in client.py

…riber_counts.py

maxalbert · 2019-04-04T12:46:11Z

integration_tests/tests/test_queries.py

+            },
+        ),
+        (
+            "unique_subscribers_count",


Why was this added twice? It seems to be an exact copy of the previous one?

DOH! Looking at what I thought was one previous test case, I assumed it was the input arguments and the expected output arguments. But on closer inspection I see it is two previous test cases and only the input arguments are required. I've removed the duplicate entry now and re-committed & pushed to github

maxalbert · 2019-04-04T12:49:39Z

Thanks for the changes @OwlHute. This looks (almost) good, but we need a changelog entry please - see the checkbox in the PR template at the top.

(Ideally we'd also want a Github issue that this PR closes, but we can add that separately.)

OwlHute · 2019-04-04T12:52:19Z

I have a horrible feeling I may have morphed the name "unique_subscriber_counts" to "unique_subscribers_count" in the added test - Checking now ..

OwlHute · 2019-04-04T12:56:59Z

Fixed!

maxalbert · 2019-04-04T14:03:14Z

flowclient/flowclient/client.py

+
+
+def unique_subscriber_counts(
+    start_date: str, stop_date: str, aggregation_unit: str


Sorry, I missed this earlier: this parameter should actually be named end_date rather than stop_date. (This was caught by the most recent integration test you added, which is why they are currently failing.)

maxalbert · 2019-04-04T14:24:27Z

This looks good now, as far as I can tell. Happy with the test coverage @greenape?

greenape · 2019-04-04T15:08:47Z

Hm. Looks like needs a case added to one or both of test_get_query_params or test_get_query_kind in integration tests, in order to hit the get_obj_type method.

maxalbert · 2019-04-05T08:14:20Z

@OwlHute Thanks for adding the extra test. It failed because the assert statement contained a hard-coded "daily_location" but your new test was against unique_subscriber_counts. I just pushed a change to fix this, so hopefully it will all pass now.

…ise the action handlers (rather than specific queries)

maxalbert · 2019-04-05T13:36:39Z

I'm happy to merge this now. The code coverage is still every so slightly reduced, but if I'm interpreting the codecov reports correctly then this seems to be a code path that's also not hit by other queries, so I'm happy to deal with this separately.

maxalbert · 2019-04-05T13:43:12Z

FYI, I have removed the tests in test_action_get_params.py and test_action_get_query_kind.py again because they are not meant to test specific queries (but rather the action handler functions), see #584.

Test coverage seems to be sufficient now, apart from a small drop that will be investigated separately

OwlHute added 2 commits April 3, 2019 13:15

Initial commit

fae25b1

Initial commit of remaining files

a5e0b9b

OwlHute requested review from greenape and maxalbert April 3, 2019 12:33

maxalbert reviewed Apr 3, 2019

View reviewed changes

flowclient/flowclient/client.py Show resolved Hide resolved

greenape added enhancement New feature or request FlowAPI Issues related to the FlowKit API FlowClient Issues related to FlowClient FlowMachine Issues related to FlowMachine labels Apr 3, 2019

After black format tweak

98dee9b

OwlHute added 2 commits April 4, 2019 08:27

Merge branch 'master' into unique_subscriber_counts

e568e26

Fixed test query schema file

1a91feb

Fix order of nested keys, too

73abcbf

greenape previously requested changes Apr 4, 2019

View reviewed changes

OwlHute added 3 commits April 4, 2019 11:10

Temporarily remove example from unique_subscribers_count entrypoint h…

b2de50a

…eader in client.py

Add test case in test_queries.py & fix header comment in unique_subsc…

f82bdb3

…riber_counts.py

Merge branch 'master' into unique_subscriber_counts

93b47c0

maxalbert reviewed Apr 4, 2019

View reviewed changes

Fix entrypoint name in test_query.py

9bbf837

OwlHute added 3 commits April 4, 2019 14:23

Remove duplicate test case from test_queries.py

e7ce319

Add CHANGELOG.md entry

8692af5

Merge branch 'master' into unique_subscriber_counts

4407727

maxalbert reviewed Apr 4, 2019

View reviewed changes

Change stop_date to end_date in client.py

f5de8bf

OwlHute and others added 4 commits April 5, 2019 08:07

Merge branch 'master' into unique_subscriber_counts

fc8d035

Add test case to test_action_get_params.py

0dfe136

Add test case to test_action_get_query_kind.py

82ce80f

Fix hard-coded query_kind to make new test pass

bbd6f90

Remove unique_subscriber_counts from tests that are supposed to exerc…

19f6f6f

…ise the action handlers (rather than specific queries)

Merge branch 'master' into unique_subscriber_counts

0329625

maxalbert added the ready-to-merge Label indicating a PR is OK to automerge label Apr 5, 2019

maxalbert approved these changes Apr 5, 2019

View reviewed changes

Merge branch 'master' into unique_subscriber_counts

3e50c9b

maxalbert merged commit 5c283c6 into master Apr 5, 2019

maxalbert deleted the unique_subscriber_counts branch April 5, 2019 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unique subscriber counts #562

Unique subscriber counts #562

OwlHute commented Apr 3, 2019 •

edited by maxalbert

Loading

maxalbert Apr 3, 2019

OwlHute Apr 3, 2019

maxalbert Apr 3, 2019

maxalbert commented Apr 3, 2019

maxalbert commented Apr 3, 2019

codecov bot commented Apr 4, 2019 •

edited

Loading

OwlHute commented Apr 4, 2019 •

edited

Loading

greenape commented Apr 4, 2019

maxalbert commented Apr 4, 2019

maxalbert commented Apr 4, 2019

OwlHute commented Apr 4, 2019 •

edited

Loading

greenape left a comment

greenape Apr 4, 2019

OwlHute Apr 4, 2019

maxalbert Apr 4, 2019 •

edited

Loading

greenape Apr 4, 2019

greenape commented Apr 4, 2019

maxalbert Apr 4, 2019

OwlHute Apr 4, 2019

maxalbert commented Apr 4, 2019

OwlHute commented Apr 4, 2019

OwlHute commented Apr 4, 2019

maxalbert Apr 4, 2019

maxalbert commented Apr 4, 2019

greenape commented Apr 4, 2019

maxalbert commented Apr 5, 2019

maxalbert commented Apr 5, 2019

maxalbert commented Apr 5, 2019

	Return the underlying flowmachine daily_location object.
	Return the underlying flowmachine UniqueSubscriberCounts object.



		def unique_subscriber_counts(
		start_date: str, stop_date: str, aggregation_unit: str

Unique subscriber counts #562

Unique subscriber counts #562

Conversation

OwlHute commented Apr 3, 2019 • edited by maxalbert Loading

I have:

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxalbert commented Apr 3, 2019

maxalbert commented Apr 3, 2019

codecov bot commented Apr 4, 2019 • edited Loading

Codecov Report

OwlHute commented Apr 4, 2019 • edited Loading

greenape commented Apr 4, 2019

maxalbert commented Apr 4, 2019

maxalbert commented Apr 4, 2019

OwlHute commented Apr 4, 2019 • edited Loading

greenape left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxalbert Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greenape commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxalbert commented Apr 4, 2019

OwlHute commented Apr 4, 2019

OwlHute commented Apr 4, 2019

Choose a reason for hiding this comment

maxalbert commented Apr 4, 2019

greenape commented Apr 4, 2019

maxalbert commented Apr 5, 2019

maxalbert commented Apr 5, 2019

maxalbert commented Apr 5, 2019

OwlHute commented Apr 3, 2019 •

edited by maxalbert

Loading

codecov bot commented Apr 4, 2019 •

edited

Loading

OwlHute commented Apr 4, 2019 •

edited

Loading

OwlHute commented Apr 4, 2019 •

edited

Loading

maxalbert Apr 4, 2019 •

edited

Loading