Skip to content

Commit

Permalink
Append correct paths for org units (#220)
Browse files Browse the repository at this point in the history
Relates to DHIS2-5987

This commit covers some edge cases for "Open as map" feature and contains:
- Fix for generating correct paths for `ou` dimension items (with `/` prefix) and supplying them to current analytical object in user data store
- Fix for generating correct parentGraphMap *from* current analytical object
- Fix for generating complete correct parentGraphMap *for* current analytical object
- Fix for generating correct parentGraphMap value for root organisation unit upon orgunit selection in `OrgUnitDimension` dialog.
- Tests

P.s. I had to reimplement `removeLastPathSegment` function which was earlier imported from `@dhis2/d2-ui-org-unit-dialog` component, since importing causes circular dependency error (presumably coming from d2).
  • Loading branch information
neeilya committed Mar 6, 2019
1 parent f7f0218 commit d50f634
Show file tree
Hide file tree
Showing 9 changed files with 140 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@ import DialogTitle from '@material-ui/core/DialogTitle';
import i18n from '@dhis2/d2-i18n';
import PropTypes from 'prop-types';
import { colors } from 'analytics-shared';

import {
OrgUnitSelector,
userOrgUnits,
removeOrgUnitLastPathSegment,
} from '@dhis2/d2-ui-org-unit-dialog';
import { OrgUnitSelector, userOrgUnits } from '@dhis2/d2-ui-org-unit-dialog';

import {
sGetUiItemsByDimension,
Expand Down Expand Up @@ -46,6 +41,7 @@ import {
getGroupsFromIds,
sortOrgUnitLevels,
transformOptionsIntoMetadata,
removeOrgUnitLastPathSegment,
} from '../../../../modules/orgUnitDimensions';

import { FIXED_DIMENSIONS } from '../../../../modules/fixedDimensions';
Expand Down Expand Up @@ -106,10 +102,18 @@ export class OrgUnitDimension extends Component {
};

addOrgUnitPathToParentGraphMap = orgUnit => {
const path = removeOrgUnitLastPathSegment(orgUnit.path);
let value;

if ('/' + orgUnit.id === orgUnit.path) {
// root org unit case
value = '';
} else {
const path = removeOrgUnitLastPathSegment(orgUnit.path);
value = path[0] === '/' ? path.substr(1) : path;
}

this.props.acAddParentGraphMap({
[orgUnit.id]: path[0] === '/' ? path.substr(1) : path,
[orgUnit.id]: value,
});
};

Expand Down
66 changes: 61 additions & 5 deletions packages/app/src/modules/__tests__/currentAnalyticalObject.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {
appendCompleteParentGraphMap,
appendDimensionItemNamesToAnalyticalObject,
appendPathsToOrgUnits,
getPathForOrgUnit,
prepareCurrentAnalyticalObject,
removeUnnecessaryAttributesFromAnalyticalObject,
} from '../currentAnalyticalObject';
Expand Down Expand Up @@ -64,6 +66,37 @@ describe('currentAnalyticalObject', () => {
};
});

describe('getPathForOrgUnit', () => {
it('generates path for orgunit using ui.parentGraphMap', () => {
const orgUnit = { id: 'SOME_ID' };
const parentGraphMap = { SOME_ID: 'ORG_UNIT/SUB_ORG_UNIT' };
const expectedPath = '/ORG_UNIT/SUB_ORG_UNIT/SOME_ID';

expect(getPathForOrgUnit(orgUnit, parentGraphMap)).toEqual(
expectedPath
);
});

it('handles root org unit case', () => {
const orgUnit = { id: 'ROOT_SIERRA_LEONE_ORG_UNIT' };
const parentGraphMap = {
ROOT_SIERRA_LEONE_ORG_UNIT: '',
};
const expectedPath = '/ROOT_SIERRA_LEONE_ORG_UNIT';

expect(getPathForOrgUnit(orgUnit, parentGraphMap)).toEqual(
expectedPath
);
});

it('returns undefined if parentGraphMap does not contain specified parent path', () => {
const orgUnit = 'USER_ORG_UNIT_CHILDREN';
const parentGraphMap = {};

expect(getPathForOrgUnit(orgUnit, parentGraphMap)).toBeUndefined();
});
});

describe('appendPathsToOrgUnits', () => {
it('appends org unit paths to current analytical object', () => {
const expected = {
Expand All @@ -74,11 +107,11 @@ describe('currentAnalyticalObject', () => {
items: [
{
id: 'qhqAxPSTUXp',
path: 'ImspTQPwCqd',
path: '/ImspTQPwCqd/qhqAxPSTUXp',
},
{
id: 'Vth0fbpFcsO',
path: 'ImspTQPwCqd',
path: '/ImspTQPwCqd/Vth0fbpFcsO',
},
],
},
Expand Down Expand Up @@ -186,8 +219,27 @@ describe('currentAnalyticalObject', () => {
});
});

describe('appendCompleteParentGraphMap', () => {
it('appends complete parent graph map property', () => {
const parentGraphMap = {
SOME_ORG_UNIT_ID: 'SOME_ORG_UNIT_PARENT',
};
const expected = {
...mockCurrent,
parentGraphMap: {
...mockCurrent.parentGraphMap,
SOME_ORG_UNIT_ID: 'SOME_ORG_UNIT_PARENT',
},
};

expect(
appendCompleteParentGraphMap(mockCurrent, { parentGraphMap })
).toEqual(expected);
});
});

describe('prepareCurrentAnalyticalObject', () => {
it('appends org unit paths, dimension item names and removes attributes and ', () => {
it('prepares current analytical object for user data store', () => {
const expected = {
key: 'value',
columns: [
Expand All @@ -201,19 +253,23 @@ describe('currentAnalyticalObject', () => {
],
},
],
parentGraphMap: {
qhqAxPSTUXp: 'ImspTQPwCqd',
Vth0fbpFcsO: 'ImspTQPwCqd',
},
filters: [
{
dimension: 'ou',
items: [
{
id: 'qhqAxPSTUXp',
name: 'Koinadugu',
path: 'ImspTQPwCqd',
path: '/ImspTQPwCqd/qhqAxPSTUXp',
},
{
id: 'Vth0fbpFcsO',
name: 'Kono',
path: 'ImspTQPwCqd',
path: '/ImspTQPwCqd/Vth0fbpFcsO',
},
],
},
Expand Down
21 changes: 21 additions & 0 deletions packages/app/src/modules/__tests__/orgUnitDimension.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getGroupsFromIds,
sortOrgUnitLevels,
transformOptionsIntoMetadata,
removeOrgUnitLastPathSegment,
} from '../orgUnitDimensions';

describe('isLevelId', () => {
Expand Down Expand Up @@ -315,3 +316,23 @@ describe('transformOptionsIntoMetadata', () => {
});
});
});

describe('removeOrgUnitLastPathSegment', () => {
it('handles a root path', () => {
const path = '/';

expect(removeOrgUnitLastPathSegment(path)).toEqual(path);
});

it('handles a path with single segment', () => {
const path = '/abc';

expect(removeOrgUnitLastPathSegment(path)).toEqual(path);
});

it('handles a path with multiple segments', () => {
const path = 'ABC/def/GHI';

expect(removeOrgUnitLastPathSegment(path)).toEqual('ABC/def');
});
});
4 changes: 2 additions & 2 deletions packages/app/src/modules/__tests__/ui.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ describe('ui', () => {
items: [
{
id: 'qhqAxPSTUXp',
path: 'ImspTQPwCqd',
path: '/ImspTQPwCqd',
},
{
id: 'Vth0fbpFcsO',
path: 'ImspTQPwCqd',
path: '/ImspTQPwCqd',
},
],
},
Expand Down
25 changes: 24 additions & 1 deletion packages/app/src/modules/currentAnalyticalObject.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,20 @@
import { FIXED_DIMENSIONS } from './fixedDimensions';
import { getDimensionIdsByAxis, getInverseLayout } from './layout';

export const getPathForOrgUnit = (orgUnit, parentGraphMap) => {
if (parentGraphMap[orgUnit.id] === undefined) {
return undefined;
}

// if this is root org unit then in parentGraphMap object
// it has empty string as value and id as key
if (parentGraphMap[orgUnit.id] === '') {
return '/' + orgUnit.id;
}

return '/' + parentGraphMap[orgUnit.id] + '/' + orgUnit.id;
};

export const appendPathsToOrgUnits = (current, ui) => {
const ouId = FIXED_DIMENSIONS.ou.id;
const dimensionIdsByAxis = getDimensionIdsByAxis(current);
Expand All @@ -18,7 +32,7 @@ export const appendPathsToOrgUnits = (current, ui) => {
...dimension,
items: dimension.items.map(item => ({
...item,
path: parentGraphMap[item.id],
path: getPathForOrgUnit(item, parentGraphMap),
})),
})),
};
Expand Down Expand Up @@ -51,12 +65,21 @@ export const appendDimensionItemNamesToAnalyticalObject = (
};
};

export const appendCompleteParentGraphMap = (current, { parentGraphMap }) => ({
...current,
parentGraphMap: {
...current.parentGraphMap,
...parentGraphMap,
},
});

export const prepareCurrentAnalyticalObject = (current, metadata, ui) => {
let result;

result = removeUnnecessaryAttributesFromAnalyticalObject(current);
result = appendDimensionItemNamesToAnalyticalObject(result, metadata);
result = appendPathsToOrgUnits(result, ui);
result = appendCompleteParentGraphMap(result, ui);

return result;
};
9 changes: 9 additions & 0 deletions packages/app/src/modules/orgUnitDimensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,12 @@ export const transformOptionsIntoMetadata = (
metadata: result,
};
};

export const removeOrgUnitLastPathSegment = path => {
// if root path, then return unprocessed path
if (path.match(/\//g).length === 1) {
return path;
}

return path.substr(0, path.lastIndexOf('/'));
};
10 changes: 9 additions & 1 deletion packages/app/src/modules/ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { isYearOverYear } from './chartTypes';
import { getOptionsFromVisualization } from './options';
import { BASE_FIELD_YEARLY_SERIES } from './fields/baseFields';
import { pieLayoutAdapter, yearOverYearLayoutAdapter } from './layoutAdapters';
import { removeOrgUnitLastPathSegment } from './orgUnitDimensions';

const peId = FIXED_DIMENSIONS.pe.id;

Expand Down Expand Up @@ -91,7 +92,14 @@ export const getParentGraphMapFromVisualization = vis => {
ouDimension.items
.filter(orgUnit => orgUnit.path)
.forEach(orgUnit => {
parentGraphMap[orgUnit.id] = orgUnit.path;
if ('/' + orgUnit.id === orgUnit.path) {
// root org unit case
parentGraphMap[orgUnit.id] = '';
} else {
const path = removeOrgUnitLastPathSegment(orgUnit.path);
parentGraphMap[orgUnit.id] =
path[0] === '/' ? path.substr(1) : path;
}
});

return parentGraphMap;
Expand Down
4 changes: 1 addition & 3 deletions packages/app/src/reducers/__tests__/ui.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@ describe('reducer: ui', () => {
...ui.DEFAULT_UI,
parentGraphMap: {
...ui.DEFAULT_UI.parentGraphMap,
[settings.rootOrganisationUnit.id]: `/${
settings.rootOrganisationUnit.id
}`,
[settings.rootOrganisationUnit.id]: '',
},
itemsByDimension: {
...ui.DEFAULT_UI.itemsByDimension,
Expand Down
2 changes: 1 addition & 1 deletion packages/app/src/reducers/ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ export default (state = DEFAULT_UI, action) => {
},
parentGraphMap: {
...DEFAULT_UI.parentGraphMap,
[rootOrganisationUnit.id]: `/${rootOrganisationUnit.id}`,
[rootOrganisationUnit.id]: '',
},
};
case TOGGLE_UI_RIGHT_SIDEBAR_OPEN:
Expand Down

0 comments on commit d50f634

Please sign in to comment.