2028 - Sort heading for CSV Export #2053

rowasc · 2017-09-12T01:37:54Z

This pull request makes the following changes:

Respects survey id, survey stage, priority number, alphabetical priority (in that order)
Important: For fields that are part of the post itself, including title and description, we do not do stage and priority sorting. They are common fields and they are sorted alphabetically, before any custom field is shown in the CSV
The sorting procedure is:
All native fields go first
After native fields
--- group by survey ID and stage ID
--- sort groups by numerical key (ie: form id =2 + stage id = 2 go after form id 1, stage id 2)
--- sort by priority inside each group. if priority is the same, fall back to alphabetical order
--- flatten, attach to native fields
--- get all records exported with the correct field order by iterating the heading attributes for each record and printing if there is a value.

TODO

Group and sort with survey (so that if you have 4 fields, two in each survey, they are next to each other and not just sorted by priority + stage which would usually match p1 - p1 , s1-s1, p2-p1,p2-p1, etc)

Test checklist:

Fixes #2028

Ping @ushahidi/platform

… in the post itself. - Missing support for multi value fields - needs cleanup & testing - - Works fine so far with single value fields

… not working

… correctly for regular non-custom fields

… , making it easier to read

rjmackay

I'll caveat this with the fact I'm pretty tired. But this looks pretty solid so far. Theres some messy data wrangling, but I think thats mostly due to our messy/mismatched data structure, not your code.

…ority. Modified Base.yml to make alphabetical sort more obvious

…r as in the Formatter

rowasc · 2017-09-12T14:43:40Z

Some data that helped me testing this to verify the heading:
Native fields:
author_email,author_realname,color,completed_stages.0,completed_stages.1,contact_id,content,created,form_id,form_name,id,locale,message_id,parent_id,post_date,published_to.0,sets.0,slug,source,status,tags.0,tags.1,title,type,updated,user_id
Attributes:
"Last Location (point).lat","Last Location (point).lon","Test varchar.0","Test varchar.1",Categories.0,Categories.1,"Geometry test.0","Second Point.lat","Second Point.lon",Status.0,Links.0,Links.1,"Person Status.0","Last Location.0","Test Field Level Locking 3.0","Test Field Level Locking 4.0","Test Field Level Locking 5.0","A Test Field Level Locking 7.0","Test Field Level Locking 6.0"

Attributes, data for sorting
"Last Location (point).lat","Last Location (point).lon", => no form id, stage 1 , priority 5
"Test varchar.0","Test varchar.1", => form id = 1, stage 1, priority 1
Categories.0,Categories.1,=> form id => 1, stage => 1, priority =>3
"Geometry test.0" => form id = 1, stage 1, priority 5
"Second Point.lat","Second Point.lon" => form id = 1, stage 1, priority 5
Status.0 => form id = 1, stage 1, priority 5
Links.0, Links.1 => form id = 1, stage 1, priority 7
"Person Status.0" => form id = 2, stage 2, priority 5
"Last Location.0" => form id = 4, stage 1, priority 5
"Test Field Level Locking 3.0" => form id = 4, stage 6, priority 0
"Test Field Level Locking 4.0" => form id = 4, stage 6, priority 0
"Test Field Level Locking 5.0" => form id = 4, stage 6, priority 0
"A Test Field Level Locking 7.0" => form id = 4, stage 7, priority 0
"Test Field Level Locking 6.0" => form id = 4, stage 7, priority 0

willdoran

Great use of comments
Very good function prototypes

I've read through the sorting, other than code cleaning suggestions, I'm not sure of a better way to sort the data. I'm going to go back through it again. In the mean time, how well does it handle large datasets?

willdoran · 2017-09-12T15:23:58Z

application/classes/Ushahidi/Formatter/Post/CSV.php

+		foreach ($fields as $fieldKey => $fieldAttr) {
+			if (!is_array($fieldAttr)) {
+				$headingResult[$fieldKey] = $fieldAttr;
+			} else if (is_array($fieldAttr) && isset($fieldAttr['nativeField'])){


I think you can drop the is_array check because isset will test anything for the presence of a key and the previous if has already confirmed that this must be an array to reach this line.

willdoran · 2017-09-12T15:31:20Z

application/classes/Ushahidi/Formatter/Post/CSV.php

+			 * uasort is used here to preserve the associative array keys when they are sorted
+			 */
+			uasort($attributeKeys, function ($item1, $item2) {
+				if ($item1['priority'] == $item2['priority']){


I think we can use strict === here since we expect the type to be the same.

yeah makes sense.

willdoran · 2017-09-12T15:36:48Z

application/classes/Ushahidi/Formatter/Post/CSV.php

+		/**
+		 * Separate by fields that have custom priority and fields that do not have custom priority assigned
+		 */
+		foreach ($fields as $fieldKey => $fieldAttr) {


For readability could you move the (non)priority splitting into a separate function?

Am I missing this bit? I meant moving 112-119 to its own function just to make this a top level function. Sorry might have not been clear enough, my bad.

ah! I just moved the specific bit with nativeFields :| Sorry about that. I will ping you in a few minutes then with that update

rowasc · 2017-09-12T19:52:19Z

Some example runs from postman:

2028 Branch

605 posts export -> 95.75kb -> 8529ms
605 posts export -> 95.75kb -> 8584ms
605 posts export -> 95.75kb -> 7427ms

Develop branch

605 posts export -> 93.95kb -> 8096ms
605 posts export -> 93.95kb -> 7789ms
605 posts export -> 93.95kb -> 8371ms

The size difference accounts for fields exported, column diferences (tags, sets and completed_stages column is only always present in the new csv export)
I'm still checking but we should be OK in terms of perf changes.

rowasc · 2017-09-12T20:03:55Z

@willdoran review feedback was addressed. Can you check? Thanks!

rowasc · 2017-09-13T02:43:01Z

Results from exporting 4510 posts in 2028 and develop branches.

2028 Branch

4510 posts export -> 635.1 kb -> 41155ms
4510 posts export -> 635.1 kb -> 39775ms
4510 posts export -> 635.1 kb -> 40261ms
4510 posts export -> 635.1 kb -> 41636ms
4510 posts export -> 635.1 kb -> 43092ms

Develop branch

4510 posts export -> 621.86 kb -> 45362ms
4510 posts export -> 621.86 kb -> 46351ms
4510 posts export -> 621.86 kb -> 47052ms
4510 posts export -> 621.86 kb -> 39440ms

willdoran · 2017-09-13T19:31:48Z

Yep looks good.

This reverts commit 3d98d04.

This reverts commit 3479d58.

rowasc added 17 commits September 9, 2017 18:22

Sorted headers

4fecb15

- Does not sort Title & Content fields wihch are default fields saved…

dd68b18

… in the post itself. - Missing support for multi value fields - needs cleanup & testing - - Works fine so far with single value fields

Clarification regarding sort order and procedure w/stages

b392c34

2028 - Working with multivalue fields - needs cleanup . Related posts…

dc63fc4

… not working

Fix csv headers for multi post examples

7c52206

Fix csv headers for multi post examples

fd12e3f

Refactored header generation with new sorting functions

933e06f

2028 working with lat/lon.

302e0f6

2028 working with lat/lon.

6dac97c

2028 - Working with tags, categories, and handling multi value fields…

18c5902

… correctly for regular non-custom fields

2028 - Working with tags, categories, and handling multi value fields…

c4b0fdd

… correctly for regular non-custom fields

2028 - Working with tags, categories, and handling multi value fields…

aba6282

… correctly for regular non-custom fields

2028 - Working with tags, categories, and handling multi value fields…

9188480

… correctly for regular non-custom fields

2028 - Working with tags, categories, and handling multi value fields…

ef4fec9

… correctly for regular non-custom fields

2028 - Working with tags, categories, and handling multi value fields…

c858a44

… correctly for regular non-custom fields

2028 - Working with tags, categories, and handling multi value fields…

9724c89

… correctly for regular non-custom fields

2028 - Refactored to have a dedicated getRecordValue private function…

c22ad0a

… , making it easier to read

rowasc added the Stage: In Review label Sep 12, 2017

rowasc added 6 commits September 11, 2017 23:12

Merge branch 'develop' into 2028

3a60c2e

2028 - add a base test for csv headings

bfda2c6

2028 - add a base test for csv headings

6e66a99

2028 - add a base test for csv headings - fix linter

0381ded

Check that row heading and content rows have the same number of columns

01da836

Check that row heading and content rows have the same number of columns

860a414

rjmackay reviewed Sep 12, 2017

View reviewed changes

rowasc added 3 commits September 12, 2017 09:04

Improve error message in rest csv test

b310268

Added alphanumeric sort inside each stage for items with the same pri…

154edfc

…ority. Modified Base.yml to make alphabetical sort more obvious

Changed test to feature survey - stage - priority - alphabetical orde…

4f634ba

…r as in the Formatter

rowasc changed the title ~~WIP - 2028~~ 2028 - Sort heading for CSV Export Sep 12, 2017

willdoran suggested changes Sep 12, 2017

View reviewed changes

Move native to function, strict comparison for priorities

d93471b

Change priority vs non priority arrays

451e028

willdoran approved these changes Sep 13, 2017

View reviewed changes

rowasc merged commit 3d98d04 into develop Sep 14, 2017

rowasc removed the Stage: In Review label Sep 14, 2017

rowasc deleted the 2028 branch September 14, 2017 12:17

rowasc added a commit that referenced this pull request Sep 14, 2017

Revert " 2028 - Sort heading for CSV Export (#2053)"

d85e6fc

This reverts commit 3d98d04.

rowasc mentioned this pull request Sep 14, 2017

Revert " 2028 - Sort heading for CSV Export" #2058

Merged

rowasc added a commit that referenced this pull request Sep 14, 2017

Revert " 2028 - Sort heading for CSV Export (#2053)" (#2058)

3479d58

This reverts commit 3d98d04.

rowasc added a commit that referenced this pull request Sep 14, 2017

Revert "Revert " 2028 - Sort heading for CSV Export (#2053)" (#2058)"

1a2dd9d

This reverts commit 3479d58.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2028 - Sort heading for CSV Export #2053

2028 - Sort heading for CSV Export #2053

rowasc commented Sep 12, 2017 •

edited by willdoran

Loading

rjmackay left a comment

rowasc commented Sep 12, 2017 •

edited

Loading

willdoran left a comment

willdoran Sep 12, 2017

willdoran Sep 12, 2017

rowasc Sep 12, 2017

willdoran Sep 12, 2017

willdoran Sep 12, 2017

rowasc Sep 12, 2017

rowasc commented Sep 12, 2017 •

edited

Loading

rowasc commented Sep 12, 2017 •

edited

Loading

rowasc commented Sep 13, 2017 •

edited

Loading

willdoran commented Sep 13, 2017

2028 - Sort heading for CSV Export #2053

2028 - Sort heading for CSV Export #2053

Conversation

rowasc commented Sep 12, 2017 • edited by willdoran Loading

TODO

rjmackay left a comment

Choose a reason for hiding this comment

rowasc commented Sep 12, 2017 • edited Loading

willdoran left a comment

Choose a reason for hiding this comment

willdoran Sep 12, 2017

Choose a reason for hiding this comment

willdoran Sep 12, 2017

Choose a reason for hiding this comment

rowasc Sep 12, 2017

Choose a reason for hiding this comment

willdoran Sep 12, 2017

Choose a reason for hiding this comment

willdoran Sep 12, 2017

Choose a reason for hiding this comment

rowasc Sep 12, 2017

Choose a reason for hiding this comment

rowasc commented Sep 12, 2017 • edited Loading

rowasc commented Sep 12, 2017 • edited Loading

rowasc commented Sep 13, 2017 • edited Loading

willdoran commented Sep 13, 2017

rowasc commented Sep 12, 2017 •

edited by willdoran

Loading

rowasc commented Sep 12, 2017 •

edited

Loading

rowasc commented Sep 12, 2017 •

edited

Loading

rowasc commented Sep 12, 2017 •

edited

Loading

rowasc commented Sep 13, 2017 •

edited

Loading