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

Support multiple roles on a contact type #86

Merged
merged 16 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Property | Type | Description
`contact_types.replacement_property` | Property | Defines how this `contact_type` is described when being replaced. The `property_name` is always `replacement`. See [ConfigProperty](#ConfigProperty).
`contact_types.place_properties` | Array<ConfigProperty> | Defines the attributes which are collected and set on the user's created place. See [ConfigProperty](#ConfigProperty).
`contact_types.contact_properties` | Array<ConfigProperty> | Defines the attributes which are collected and set on the user's primary contact doc. See [ConfigProperty](#ConfigProperty).
`contact_types.user_roles_property` | ConfigProperty | Defines the property used for specifying user role(s) when creating users of this contact type. Add flexibility to choose role(s) when there are multiple roles available at this level of your hierarchy. See [ConfigProperty](#ConfigProperty).
Copy link
Member

Choose a reason for hiding this comment

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

I think it is important that we somehow capture the "allowed roles" at each level. Currently, this feature supports free-text input but what if somebody uses the wrong case? What if they use a CHW role for a supervisor user (this can actually result in quite serious bugs)? What if there are trailing spaces on the role, which is super common when working with CSVs?

Ideally, I think the best user experience for this feature would be a set of checkboxes like in the CHT today (see screenshot). You don't need to implement that UI here in this PR, we can start with free-text; but if we capture the "allowed roles" and validate them, I think that's a good start.

image

There are probably a few ways to approach this. But you'll want to validate this propery like the other property types. You'll probably end up with something like argument: ["role1", "role2"].

Copy link
Member

@kennsippell kennsippell Mar 12, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to use it, but since select_multiple only accept parameter to be object for the moment (not simple array).

Got a simple role validator that can do the job, maybe the time to complete select_multiple feature.

`logoBase64` | Image in base64 | Logo image for your project

#### ConfigProperty
Expand Down
1 change: 1 addition & 0 deletions src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export type ContactType = {
replacement_property: ContactProperty;
place_properties: ContactProperty[];
contact_properties: ContactProperty[];
user_roles_property?: ContactProperty;
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
};

export type HierarchyConstraint = {
Expand Down
9 changes: 9 additions & 0 deletions src/public/place/bulk_create_form.html
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,15 @@
<td>Property on primary contact</td>
</tr>
{% endfor %}

{% if contactType.user_roles_property != null %}
<tr>
<td>{{ contactType.user_roles_property.friendly_name }}</td>
<td>{{ contactType.user_roles_property.required }}</td>
<td>{{ contactType.user_roles_property.description | default: 'Role(s) of the user, use when there are multiple roles available at this level of your hierarchy. <br> <i>eg: rcom or supervisor+stock_manager </i> <br> (separate multiple roles with <b>+</b> or leave blank to use the default role of this level.)' | newline_to_br }}
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
</td>
</tr>
{% endif %}
</tbody>
</table>
</div>
Expand Down
7 changes: 7 additions & 0 deletions src/public/place/create_form.html
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@
%}
{% endfor %}
</section>
<section class="section is-small">
{% if contactType.user_roles_property != null %}
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
{%
include "components/contact_type_property.html" prefix="user_roles_" prop=contactType.user_roles_property
%}
{% endif %}
</section>

<div class=" field is-grouped is-grouped-right">
<div class="control">
Expand Down
13 changes: 13 additions & 0 deletions src/public/place/list.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ <h2 class="title is-4">{{contactType.friendly}}</h2>
{% for contact_property in contactType.contact_properties %}
<th id="contact_{{contact_property.property_name}}">{{contact_property.friendly_name}}</th>
{% endfor %}
{% if contactType.user_roles_property %}
<th id="user_roles_{{ contactType.user_roles_property.property_name }}">{{ contactType.user_roles_property.friendly_name }}</th>
{% endif %}
<th></th>
<th></th>
</tr>
Expand Down Expand Up @@ -57,6 +60,16 @@ <h2 class="title is-4">{{contactType.friendly}}</h2>
%}
{% endfor %}

{% if contactType.user_roles_property != null %}
{% capture propertyName %}user_roles_{{ contactType.user_roles_property.property_name }}{% endcapture %}
{%
include "components/list_cell.html"
propertyName=propertyName
property=contactType.user_roles_property
values=place.userRolesProperty
%}
{% endif %}

<td>
{% capture tag_text %}{% if place.validationErrors == empty %}{{ place.state }}{% else %}INVALID{% endif %}{% endcapture %}
{% capture tag_class %}
Expand Down
8 changes: 8 additions & 0 deletions src/services/place-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ export default class PlaceFactory {
const columnIndex = csvColumns.indexOf(hierarchyConstraint.friendly_name);
place.hierarchyProperties[hierarchyConstraint.property_name] = row[columnIndex];
}

if (contactType.user_roles_property) {
const userRolesProperty = contactType.user_roles_property;
place.userRolesProperty[userRolesProperty.property_name] = row[
csvColumns.indexOf(userRolesProperty.friendly_name)
];
}

places.push(place);
}
count++;
Expand Down
21 changes: 21 additions & 0 deletions src/services/place.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export enum PlaceUploadState {

const PLACE_PREFIX = 'place_';
const CONTACT_PREFIX = 'contact_';
const USER_ROLES_PREFIX = 'user_roles_';

export default class Place {
public readonly id: string;
Expand All @@ -46,6 +47,10 @@ export default class Place {
[key: string]: any;
};

public userRolesProperty: {
[key: string]: any;
};

public state : PlaceUploadState;

public validationErrors?: { [key: string]: string };
Expand All @@ -59,6 +64,7 @@ export default class Place {
this.hierarchyProperties = {};
this.state = PlaceUploadState.STAGED;
this.resolvedHierarchy = [];
this.userRolesProperty = {};
}

/*
Expand Down Expand Up @@ -91,6 +97,11 @@ export default class Place {
this.hierarchyProperties[propertyName] = formData[`${hierarchyPrefix}${propertyName}`];
}
}

if (this.type.user_roles_property) {
const propertyName = this.type.user_roles_property.property_name;
this.userRolesProperty[propertyName] = formData[`${USER_ROLES_PREFIX}${propertyName}`];
}
}

/**
Expand All @@ -113,6 +124,7 @@ export default class Place {
...addPrefixToPropertySet(this.hierarchyProperties, hierarchyPrefix),
...addPrefixToPropertySet(this.properties, PLACE_PREFIX),
...addPrefixToPropertySet(this.contact.properties, CONTACT_PREFIX),
...addPrefixToPropertySet(this.userRolesProperty, USER_ROLES_PREFIX),
};
}

Expand Down Expand Up @@ -220,6 +232,15 @@ export default class Place {
return username;
}

public extractUserRoles(): string[] {
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
const ROLES_SEPARATOR = '+';

const userRolesProperty = this.type.user_roles_property;
const roles = userRolesProperty ? this.userRolesProperty[userRolesProperty.property_name] : undefined;

return roles ? roles.split(ROLES_SEPARATOR).map((role: string) => role.trim()).filter(Boolean) : [];
}

public get hasValidationErrors() : boolean {
return Object.keys(this.validationErrors as any).length > 0;
}
Expand Down
19 changes: 18 additions & 1 deletion src/services/user-payload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ export class UserPayload {
public contact: string;
public fullname: string;
public phone: string;
public roles: string[];

constructor(place: Place, placeId: string, contactId: string) {
this.username = place.generateUsername();
this.password = this.generatePassword();
this.type = place.type.user_role;
this.roles = place.extractUserRoles();
this.type = this.getUserType(place);
this.place = placeId;
this.contact = contactId;
this.fullname = place.contact.name;
Expand All @@ -29,6 +31,21 @@ export class UserPayload {
this.username = `${this.username}${randomNumber.toString()}`;
}

/**
* Retrieves the user type from the place.
* If roles are present, this method returns an empty string.
* If roles are not present, it returns the user type defined.
*
* When the type is sent alongside roles to CHT, roles are ignored.
* In such cases, this method returns an empty string to prioritize roles.
*
* @param place - The place object containing creation details and type information.
* @returns A string representing the user type.
*/
public getUserType(place: Place): string {
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
return place.extractUserRoles().length > 0 ? '' : place.type.user_role;
}

private generatePassword(): string {
const LENGTH = 9; // CHT requires 8 minimum + special characters
const ELIGIBLE_CHARACTERS =
Expand Down
1 change: 1 addition & 0 deletions test/lib/retry-logic.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ describe('lib/retry-logic', () => {
};
const place = {
generateUsername: sinon.stub().returns('username'),
extractUserRoles: sinon.stub().returns([]),
type: mockSimpleContactType('string', 'bar'),
contact: { properties: {} },
};
Expand Down
9 changes: 9 additions & 0 deletions test/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,12 @@ export function expectInvalidProperties(
expect(Object.values(validationErrors as any)?.[0]).to.include(expectedError);
}
}

export const mockValidMultipleRolesContactType = (propertyType, propertyValidator: string | string[] | undefined): ContactType => {
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
const baseMock = mockValidContactType(propertyType, propertyValidator);

return {
...baseMock,
user_roles_property: mockProperty('string', undefined, 'roles'),
};
};
43 changes: 40 additions & 3 deletions test/services/upload-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { expect } from 'chai';
import sinon from 'sinon';

import { UploadManager } from '../../src/services/upload-manager';
import { mockValidContactType, mockParentPlace, mockChtSession, expectInvalidProperties } from '../mocks';
import { mockValidContactType, mockParentPlace, mockChtSession, expectInvalidProperties, mockValidMultipleRolesContactType } from '../mocks';
import PlaceFactory from '../../src/services/place-factory';
import SessionCache from '../../src/services/session-cache';
import { ChtApi, RemotePlace } from '../../src/lib/cht-api';
Expand Down Expand Up @@ -245,6 +245,38 @@ describe('upload-manager.ts', () => {
expect(chtApi.createUser.callCount).to.be.gt(2); // retried
expect(chu.uploadError).to.include('could not create user');
});

it('mock data is properly sent to chtApi (multiple roles)', async () => {
paulpascal marked this conversation as resolved.
Show resolved Hide resolved
const { fakeFormData, contactType, chtApi, sessionCache, remotePlace } = await createMocks({ supportMultipleRoles: true });
const place = await PlaceFactory.createOne(fakeFormData, contactType, sessionCache, chtApi);

const uploadManager = new UploadManager();
await uploadManager.doUpload([place], chtApi);

expect(chtApi.createPlace.calledOnce).to.be.true;
const placePayload = chtApi.createPlace.args[0][0];
expect(placePayload).to.nested.include({
'contact.contact_type': contactType.contact_type,
'contact.name': 'contact',
prop: 'foo',
name: 'Place Community Health Unit',
parent: remotePlace.id,
contact_type: contactType.name,
});
expect(chtApi.updateContactParent.calledOnce).to.be.true;
expect(chtApi.updateContactParent.args[0]).to.deep.eq(['created-place-id']);

expect(chtApi.createUser.calledOnce).to.be.true;
const userPayload = chtApi.createUser.args[0][0];
expect(userPayload).to.deep.include({
contact: 'created-contact-id',
place: 'created-place-id',
type: '',
roles: ['role1', 'role2'],
username: 'contact',
});
expect(place.isCreated).to.be.true;
});
});

async function createChu(remotePlace: RemotePlace, chu_name: string, sessionCache: any, chtApi: ChtApi) {
Expand All @@ -263,8 +295,12 @@ async function createChu(remotePlace: RemotePlace, chu_name: string, sessionCach
return chu;
}

async function createMocks() {
const contactType = mockValidContactType('string', undefined);
async function createMocks(options: {
supportMultipleRoles?: boolean;
} = {}) {
const contactType = options.supportMultipleRoles
? mockValidMultipleRolesContactType('string', undefined)
: mockValidContactType('string', undefined);
const remotePlace: RemotePlace = {
id: 'parent-id',
name: 'parent-name',
Expand All @@ -290,6 +326,7 @@ async function createMocks() {
place_prop: 'foo',
hierarchy_PARENT: remotePlace.name,
contact_name: 'contact',
...(options.supportMultipleRoles ? { user_roles_roles: 'role1+role2' } : {}),
};

return { fakeFormData, contactType, sessionCache, chtApi, remotePlace };
Expand Down
Loading