Tableau v2 #1910

WinnyTroy · 2020-09-30T13:10:58Z

Changes / Features implemented

Defines a new endpoint implementing multiple tables on Tableau repeat data collected on Onadata.
With this every repeat/nested repeat defines a new tableau table.

Steps taken to verify this change does what is intended

Confirming schema and data definitions on the new /api/v1/open-data/v2 endpoint
Add tests

An alternative solution for #1880
Part of #1885

onadata/apps/api/viewsets/tableau_viewset.py

pld · 2020-10-28T07:10:13Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- onadata/apps/api/viewsets/tableau_viewset.py  25
         

Clones added
============
- onadata/apps/api/viewsets/open_data_viewset.py  1
- onadata/apps/api/viewsets/tableau_viewset.py  1

See the complete overview on Codacy

ivermac

A few comments but it generally looks good

ivermac · 2020-11-10T05:19:45Z

onadata/apps/api/viewsets/tableau_viewset.py

+                        # Pop any list within the returned repeat data.
+                        # Lists represent a repeat group which should be in a
+                        # separate field.
+                        cleaned_data = []
+                        for data_dict in repeat_data:
+                            remove_keys = []
+                            for k, v in data_dict.items():
+                                if isinstance(v, list):
+                                    remove_keys.append(k)
+                                    flat_dict[k].extend(v)
+                            # pylint: disable=expression-not-assigned
+                            [data_dict.pop(k) for k in remove_keys]
+                            cleaned_data.append(data_dict)
+                        flat_dict[qstn_name] = cleaned_data


can we move this to function that returns cleaned_data ?

ivermac · 2020-11-10T05:22:26Z

onadata/apps/api/viewsets/tableau_viewset.py

+                        picked_choices = value.split(" ")
+                        choice_names = [
+                            question["name"] for question in qstn["children"]]
+                        list_name = qstn.get('list_name')
+                        for choice in choice_names:
+                            qstn_name = f"{list_name}_{choice}"
+
+                            if prefix:
+                                qstn_name = prefix + '_' + qstn_name
+
+                            if choice in picked_choices:
+                                flat_dict[qstn_name] = "TRUE"
+                            else:
+                                flat_dict[qstn_name] = "FALSE"


Can we move this to a function outside this function that updates flat_dict?

ivermac · 2020-11-10T05:23:57Z

onadata/apps/api/viewsets/tableau_viewset.py

+                        value_parts = value.split(' ')
+                        gps_xpath_parts = []
+                        for part in GPS_PARTS:
+                            name = f"_{qstn_name}_{part}"
+                            if prefix:
+                                name = prefix + '_' + name
+                            gps_xpath_parts.append((name, None))
+                        if len(value_parts) == 4:
+                            gps_parts = dict(
+                                zip(dict(gps_xpath_parts), value_parts))
+                            flat_dict.update(gps_parts)


Can we move this to a function outside this function?

ivermac

Getting close

ivermac · 2020-11-17T12:36:33Z

onadata/apps/api/viewsets/tableau_viewset.py

+def unpack_select_multiple_data(picked_choices, list_name,
+                                choice_names, prefix, flat_dict):
+    for choice in choice_names:
+        qstn_name = f"{list_name}_{choice}"
+
+        if prefix:
+            qstn_name = prefix + '_' + qstn_name
+
+        if choice in picked_choices:
+            flat_dict[qstn_name] = "TRUE"
+        else:
+            flat_dict[qstn_name] = "FALSE"


Let's change this function to return flat_dict - just like it's been done in line 116 - then assign the returned value to flat_dict in line 70

Made this change

ivermac · 2020-11-17T12:39:44Z

onadata/apps/api/viewsets/tableau_viewset.py

+                        gps_parts = unpack_gps_data(
+                            value, qstn_name, prefix)
+                        flat_dict.update(gps_parts)


if unpack_gps_data returns None, line 79 would trigger an error, no?

I would say no. I dont believe there could be a scenerio where it would return None. Maybe this is something new we could test

The function can only be called if the element is of "geopoint" question type. All geopoint question types are space separated strings, unless the user does not provide data for this question, for which the field is then not present in the json returned by the serializer. I could be wrong Cc: @DavisRayM

Add repeat column headers list if the headers list already exists.

…mn headers have been defined using the select multiple question. Gnerate column headers with gps headers split into 4 column headers ie. altitude, latitude, precision and longitude

- Split gps fields into separate columns - Ensure schema and data always have the same columns by utilizing the JSON of an XLS Form - Support "select_multiple" questions

Add tests

Break apart the process tableau data function into separate functions

onadata/apps/api/viewsets/tableau_viewset.py

WinnyTroy changed the title ~~Tableau nested repeat check~~ Tableau v2 Sep 30, 2020

DavisRayM reviewed Oct 13, 2020

View reviewed changes

onadata/apps/api/viewsets/tableau_viewset.py Outdated Show resolved Hide resolved

DavisRayM reviewed Oct 13, 2020

View reviewed changes

onadata/apps/api/viewsets/tableau_viewset.py Outdated Show resolved Hide resolved

WinnyTroy force-pushed the tableau_nested_repeat_check branch 2 times, most recently from 260c198 to 72e552b Compare October 13, 2020 08:50

DavisRayM force-pushed the tableau_nested_repeat_check branch from ab9f686 to 8fc3ad9 Compare October 13, 2020 11:48

WinnyTroy force-pushed the tableau_nested_repeat_check branch 2 times, most recently from ed167a7 to fe7c2ad Compare October 19, 2020 08:53

DavisRayM force-pushed the tableau_nested_repeat_check branch 7 times, most recently from 585812c to ad257ab Compare October 28, 2020 07:07

DavisRayM force-pushed the tableau_nested_repeat_check branch 3 times, most recently from 2f55c5d to 66949a2 Compare November 5, 2020 08:09

DavisRayM requested review from ivermac and ukanga November 6, 2020 06:45

WinnyTroy mentioned this pull request Nov 9, 2020

Tableau Hot fix #1888

Closed

ivermac reviewed Nov 10, 2020

View reviewed changes

WinnyTroy force-pushed the tableau_nested_repeat_check branch 4 times, most recently from dde45e9 to 5ae768a Compare November 17, 2020 06:28

ivermac suggested changes Nov 17, 2020

View reviewed changes

WinnyTroy force-pushed the tableau_nested_repeat_check branch 2 times, most recently from 7bb5906 to e14821d Compare November 18, 2020 13:57

DavisRayM and others added 12 commits January 4, 2021 09:43

Change URL route for the Open Data V2 endpoint

6f4cbb9

Return headers as a string instead of list

c7428dd

Add repeat_parent_fields to the repeat column headers list

e3fe866

Add repeat column headers list if the headers list already exists.

Return acceptable tableau column headers

cdac713

Fix failing tests

ea63ea2

Undo breaking select multiple data into separate columns, as the colu…

092facc

…mn headers have been defined using the select multiple question. Gnerate column headers with gps headers split into 4 column headers ie. altitude, latitude, precision and longitude

Fix duplicate tableau headers issue

6698f3b

Remove metadata fields from data

4444527

Ensure header is not already in column header list before adding

7e01dd1

Add PARENT_TABLE and PARENT_ID common tags

d844048

Code Cleanup

3904f67

- Split gps fields into separate columns - Ensure schema and data always have the same columns by utilizing the JSON of an XLS Form - Support "select_multiple" questions

Support gps fields within repeats

0402d51

Add tests

WinnyTroy force-pushed the tableau_nested_repeat_check branch from ded004d to 604567f Compare January 4, 2021 06:43

Add tests for tableau viewset

78896bf

Break apart the process tableau data function into separate functions

WinnyTroy force-pushed the tableau_nested_repeat_check branch from 604567f to 78896bf Compare January 4, 2021 07:17

ivermac previously approved these changes Jan 4, 2021

View reviewed changes

DavisRayM reviewed Jan 5, 2021

View reviewed changes

onadata/apps/api/viewsets/tableau_viewset.py Outdated Show resolved Hide resolved

WinnyTroy dismissed ivermac’s stale review via 1bf0306 January 5, 2021 09:09

WinnyTroy force-pushed the tableau_nested_repeat_check branch 6 times, most recently from 085441a to 4676dd3 Compare January 5, 2021 10:22

Include more tests for helper functions in new tableau viewset

049f53a

WinnyTroy force-pushed the tableau_nested_repeat_check branch from 4676dd3 to 049f53a Compare January 5, 2021 13:03

DavisRayM approved these changes Jan 5, 2021

View reviewed changes

DavisRayM merged commit 3a0154d into master Jan 5, 2021

DavisRayM deleted the tableau_nested_repeat_check branch January 5, 2021 13:38

DavisRayM mentioned this pull request Jan 20, 2021

Tag release v2.5.0 #1998

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tableau v2 #1910

Tableau v2 #1910

WinnyTroy commented Sep 30, 2020 •

edited by DavisRayM

Loading

pld commented Oct 28, 2020

ivermac left a comment

ivermac Nov 10, 2020

WinnyTroy Nov 16, 2020

ivermac Nov 10, 2020

ivermac Nov 10, 2020

ivermac left a comment

ivermac Nov 17, 2020

WinnyTroy Nov 18, 2020

ivermac Nov 17, 2020

WinnyTroy Nov 18, 2020

Tableau v2 #1910

Tableau v2 #1910

Conversation

WinnyTroy commented Sep 30, 2020 • edited by DavisRayM Loading

Changes / Features implemented

Steps taken to verify this change does what is intended

pld commented Oct 28, 2020

ivermac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivermac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinnyTroy commented Sep 30, 2020 •

edited by DavisRayM

Loading