Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose 'event_types' parameters #2632

Merged
merged 17 commits into from
Jun 3, 2020
Merged

Expose 'event_types' parameters #2632

merged 17 commits into from
Jun 3, 2020

Conversation

jc-harrison
Copy link
Member

Closes #2631

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

Exposes an event_types parameter for all queries that internally have a table/tables parameter for selecting a subset of events tables in EventsTablesUnion.

@jc-harrison jc-harrison added enhancement New feature or request FlowClient Issues related to FlowClient FlowMachine Issues related to FlowMachine FlowAPI Issues related to the FlowKit API labels Jun 2, 2020
@cypress
Copy link

cypress bot commented Jun 2, 2020



Test summary

43 0 0 0


Run details

Project FlowAuth
Status Passed
Commit d1a83ab
Started Jun 3, 2020 1:14 PM
Ended Jun 3, 2020 1:17 PM
Duration 02:34 💡
OS Linux Debian - 8.11
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #2632 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2632      +/-   ##
==========================================
+ Coverage   93.86%   93.88%   +0.02%     
==========================================
  Files         241      241              
  Lines        9401     9435      +34     
  Branches      851      852       +1     
==========================================
+ Hits         8824     8858      +34     
  Misses        448      448              
  Partials      129      129              
Flag Coverage Δ
#autoflow_unit_tests 92.99% <ø> (ø)
#flowapi_unit_tests 65.63% <ø> (ø)
#flowauth_unit_tests 84.89% <ø> (ø)
#flowclient_unit_tests 76.83% <ø> (ø)
#flowetl_unit_tests 100.00% <ø> (ø)
#flowkit_jwt_generator_unit_tests 100.00% <ø> (ø)
#flowmachine_unit_tests 91.16% <76.66%> (-0.16%) ⬇️
#integration_tests 72.03% <93.33%> (+0.08%) ⬆️
Impacted Files Coverage Δ
flowclient/flowclient/aggregates.py 95.83% <ø> (ø)
flowclient/flowclient/query_specs.py 100.00% <ø> (ø)
...erver/query_schemas/consecutive_trips_od_matrix.py 100.00% <100.00%> (ø)
...machine/core/server/query_schemas/custom_fields.py 77.58% <100.00%> (ø)
...achine/core/server/query_schemas/daily_location.py 100.00% <100.00%> (ø)
...wmachine/core/server/query_schemas/displacement.py 100.00% <100.00%> (ø)
...e/flowmachine/core/server/query_schemas/handset.py 100.00% <100.00%> (ø)
...core/server/query_schemas/location_introversion.py 100.00% <100.00%> (ø)
.../core/server/query_schemas/meaningful_locations.py 100.00% <100.00%> (ø)
...achine/core/server/query_schemas/modal_location.py 100.00% <100.00%> (ø)
... and 10 more

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 f576c06...d1a83ab. Read the comment docs.

@jc-harrison jc-harrison added the ready-to-merge Label indicating a PR is OK to automerge label Jun 2, 2020
Copy link
Member

@greenape greenape left a comment

Choose a reason for hiding this comment

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

Minor stuff then all good.

@@ -113,6 +113,7 @@ def meaningful_locations_aggregate_spec(
aggregation_unit: str,
tower_cluster_radius: float = 1.0,
tower_cluster_call_threshold: int = 0,
event_types: Union[None, List[str]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Make it consistently Optional[List[str]]?

@@ -167,6 +168,8 @@ def meaningful_locations_aggregate_spec(
more towers, and fewer clusters.
tower_cluster_call_threshold : int
Exclude towers from a subscriber's clusters if they have been used on less than this number of days.
event_types : None or list of {"calls", "sms", "mds"}, default None
Copy link
Member

Choose a reason for hiding this comment

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

Or topups?

Almost feel like worth using the str Enum trick for this.

@mergify mergify bot merged commit 279fa39 into master Jun 3, 2020
@mergify mergify bot deleted the event-types branch June 3, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request FlowAPI Issues related to the FlowKit API FlowClient Issues related to FlowClient FlowMachine Issues related to FlowMachine ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose 'event_types' parameter for all applicable queries
2 participants