Include support for repeating sections for the tableau-onadata integration #1845

WinnyTroy · 2020-07-08T13:06:09Z

Changes / Features implemented

Include support for nested repeats and nested groups for the tableau integration.

Steps taken to verify this change does what is intended

QA

Side effects of implementing this change

None

Closes #
Resolves part of #1777

faith-mutua · 2020-07-10T08:52:08Z

@WinnyTroy Data in nested groups is being downloaded to Tableau.
However, I have noted that data within nested repeat groups is not being downloaded. It's only data in the main/parent repeat group that's being downloaded. Use this sample form to test this.

faith-mutua · 2020-07-16T10:32:50Z

@WinnyTroy I have tested different scenarios and the results are as follows;

Nested repeat in a group- Data in the nested repeat group is not downloaded but data in the parent repeat group and the group is downloaded.
Nested repeat in nested group - Only data in the nested groups is downloaded. Data collected in both the parent repeat group and the nested repeat group is not downloaded.
Nested group - All data is downloaded
Nested repeat - Data is downloaded
Group in a nested repeat - Data in the group is not downloaded. Data collected in both the parent repeat group and the nested repeat group is downloaded.
nested group in a nested repeat - Data in nested groups is not downloaded. Data collected in both the parent repeat group and the nested repeat group is downloaded.

ukanga

Can you add tests, please?

ivermac

Generally looks good to me. I've added a few suggestions

ivermac · 2020-07-30T08:01:03Z

onadata/apps/api/viewsets/open_data_viewset.py

+                index=index,
+                close_tag=index_tags[1])
+                ]
+        if len(key.split('/')) > 1:


SInce we've referenced len(key.split('/')) in line 70 and 64, may be we want to set the call in a variable then call the variable on those lines.

ivermac · 2020-07-30T08:02:21Z

onadata/apps/api/viewsets/open_data_viewset.py

+            except TypeError:
+                xpaths = xpaths[0] + ('/').join(nested_key_diff)
+        else:
+            xpaths = xpaths + [nested_key.split('/')[1]]


SInce we've referenced nested_key.split('/') in line 76 and 60, may be we want to set the call in a variable then call the variable on those lines.

Alright, let me update this

ivermac · 2020-07-30T08:03:20Z

onadata/apps/api/viewsets/open_data_viewset.py

+        xpaths = [
+            '{key}{open_tag}{index}{close_tag}'.format(
+                key=key if len(key.split('/')) > 1
+                else nested_key.split('/')[0],
+                open_tag=index_tags[0],
+                index=index,
+                close_tag=index_tags[1])
+                ]


Not really sure what's going on here 😄

I've seen where it's been moved from 😄

ivermac

Getting close.

ivermac · 2020-08-11T10:29:56Z

onadata/apps/api/viewsets/open_data_viewset.py

+    def unpack_data(key, value, data=None, xpaths=None):
+        if isinstance(value, str) and data:
+            choices = value.split(" ")
+            for choice in choices:
+                xpaths = f'{key}/{choice}'
+                data[xpaths] = choice


Do we want to rename this function to something like update_data since we are updating the data dictionary in the function?

DavisRayM · 2020-08-11T13:08:56Z

onadata/apps/api/viewsets/open_data_viewset.py

+        xpaths = key + f'[{index}]/' + '/'.join(nested_key_diff)
+        return xpaths
+
+    def unpack_data(key, value, data=None, xpaths=None):


Please add a docstring for this function.... Finding it a bit hard to tell why xpaths is an argument... In what scenario would someone pass xpaths ?

Is data supposed to be optional ? Seems like the function can't run without it ?

Ah...changed those two items. Thanks for the catch

ivermac

Getting close

ivermac · 2020-08-12T06:31:48Z

onadata/apps/api/viewsets/open_data_viewset.py

+        xpaths = key + f'[{index}]/' + '/'.join(nested_key_diff)
+        return xpaths
+
+    def unpack_select_multiple_data(key, value, data):


Not really sure about the function name here since as mentioned in my previous comment, the main functionality, and correct me if I'm wrong, is to update the data variable using choices values. How about we rename it to get_updated_data and make ensure that data is returned?

... if isinstance(value, str) and data: choices = value.split(" ") for choice in choices: xpaths = f'{key}/{choice}' data[xpaths] = choice return data

Then we can then assign the data variable with result from get_updated_data like the following:

... if qstn_type == MULTIPLE_SELECT_TYPE: data = get_updated_data(xpaths, nested_val, data)

pld · 2020-08-12T08:39:19Z

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

Complexity decreasing per file
==============================
+ onadata/apps/api/viewsets/open_data_viewset.py  -1

See the complete overview on Codacy

pld · 2020-08-28T11:50:41Z

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

Complexity decreasing per file
==============================
+ onadata/apps/api/viewsets/open_data_viewset.py  -1
         

Clones added
============
- onadata/apps/api/tests/viewsets/test_open_data_viewset.py  2

See the complete overview on Codacy

WinnyTroy force-pushed the tableau_repeating_section_support branch from 9be5c9d to dee81d5 Compare July 8, 2020 13:13

WinnyTroy marked this pull request as ready for review July 9, 2020 12:22

ukanga requested changes Jul 22, 2020

View reviewed changes

WinnyTroy force-pushed the tableau_repeating_section_support branch from 481755b to f3a4b47 Compare July 27, 2020 15:23

ivermac reviewed Jul 30, 2020

View reviewed changes

WinnyTroy force-pushed the tableau_repeating_section_support branch 5 times, most recently from a78b80c to 5874767 Compare August 11, 2020 07:18

ivermac reviewed Aug 11, 2020

View reviewed changes

DavisRayM reviewed Aug 11, 2020

View reviewed changes

WinnyTroy force-pushed the tableau_repeating_section_support branch from 5874767 to b7d97e6 Compare August 11, 2020 14:15

ivermac suggested changes Aug 12, 2020

View reviewed changes

WinnyTroy force-pushed the tableau_repeating_section_support branch 3 times, most recently from 7438753 to 78d16d8 Compare August 12, 2020 08:37

ivermac previously approved these changes Aug 24, 2020

View reviewed changes

ukanga previously approved these changes Aug 27, 2020

View reviewed changes

WinnyTroy added 5 commits August 28, 2020 14:48

Include support for repeating sections

7926f50

Code cleanup

5bd62a6

Include unpack repeat functionality

f2a8733

Include tests

3bfbf38

Fix failing tests

628dd53

WinnyTroy dismissed stale reviews from ukanga and ivermac via 628dd53 August 28, 2020 11:48

WinnyTroy force-pushed the tableau_repeating_section_support branch from 78d16d8 to 628dd53 Compare August 28, 2020 11:48

DavisRayM merged commit 454e89e into master Aug 28, 2020

DavisRayM deleted the tableau_repeating_section_support branch August 28, 2020 11:53

DavisRayM mentioned this pull request Aug 31, 2020

Tag release v2.4.0 #1884

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include support for repeating sections for the tableau-onadata integration #1845

Include support for repeating sections for the tableau-onadata integration #1845

WinnyTroy commented Jul 8, 2020 •

edited

Loading

faith-mutua commented Jul 10, 2020

faith-mutua commented Jul 16, 2020

ukanga left a comment

ivermac left a comment

ivermac Jul 30, 2020

ivermac Jul 30, 2020

WinnyTroy Jul 30, 2020

ivermac Jul 30, 2020

ivermac Jul 30, 2020

ivermac left a comment

ivermac Aug 11, 2020

DavisRayM Aug 11, 2020

WinnyTroy Aug 11, 2020

ivermac left a comment

ivermac Aug 12, 2020

pld commented Aug 12, 2020

pld commented Aug 28, 2020

Include support for repeating sections for the tableau-onadata integration #1845

Include support for repeating sections for the tableau-onadata integration #1845

Conversation

WinnyTroy commented Jul 8, 2020 • edited Loading

Changes / Features implemented

Steps taken to verify this change does what is intended

Side effects of implementing this change

faith-mutua commented Jul 10, 2020

faith-mutua commented Jul 16, 2020

ukanga left a 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

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

ivermac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pld commented Aug 12, 2020

pld commented Aug 28, 2020

WinnyTroy commented Jul 8, 2020 •

edited

Loading