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

Optmise Disbursement layers on map #35516

Merged
merged 12 commits into from
Dec 17, 2024
Merged

Conversation

ajeety4
Copy link
Contributor

@ajeety4 ajeety4 commented Dec 12, 2024

Product Description

Updates logic to create a single disbursement layer for lines connecting cases and mobile workers on map instead of one layer for each line. This significantly improves the map performance for higher assignments(>1k).

Technical Summary

Ticket

Feature Flag

Microplanning.

Safety Assurance

Safety story

Behind a feature flag and changes only relates to disbursement. Tested on local.

Automated test coverage

QA Plan

None

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@ajeety4 ajeety4 added product/feature-flag Change will only affect users who have a specific feature flag enabled product/invisible Change has no end-user visible impact labels Dec 12, 2024
@ajeety4 ajeety4 changed the title Ay/optmise disbursement layer creation Optmise Disbursement layers on map Dec 12, 2024
@ajeety4 ajeety4 marked this pull request as ready for review December 12, 2024 16:13
@ajeety4 ajeety4 requested review from zandre-eng, mkangia, Charl1996 and kaapstorm and removed request for zandre-eng and mkangia December 12, 2024 16:13
@Charl1996
Copy link
Contributor

Charl1996 commented Dec 16, 2024

@ajeety4
Look good, but just one suggestion (feel free to evaluate and dispute it):

The handleDisbursementResults function iterates over all the users and cases, then the connectUserWithCasesOnMap iterates over all the users and cases again.

How about the following (note that I haven't tested it) - I removed the userToCasesList list to immediately construct the disbursementLinesSource (which would have been done in connectUserWithCasesOnMap), since we already have all the data we need to do so and we're skipping having to loop over all users/cases again:

        self.handleDisbursementResults = function (result) {
            // Clean stale disbursement results
            mapModel.removeDisbursementLayer();

            let groupId = 0;
            let disbursementLinesSource = [];
            Object.keys(result).forEach((userId) => {
                const user = mapModel.userMapItems().find((userModel) => {return userModel.itemId === userId;});
                mapModel.caseGroupsIndex[userId] = {groupId: groupId, item: user};

                let userToCases = {'user': user, 'cases': []};
                mapModel.caseMapItems().forEach((caseModel) => {
                    if (result[userId].includes(caseModel.itemId)) {
                        const lineCoordinates = [
                            [user.itemData.coordinates.lng, user.itemData.coordinates.lat],
                            [caseModel.itemData.coordinates.lng, caseModel.itemData.coordinates.lat],
                        ];
                        disbursementLinesSource.features.push(
                            {
                                type: 'Feature',
                                properties: {},
                                geometry: { type: 'LineString', coordinates: lineCoordinates },
                            }
                        );
                        
                        mapModel.caseGroupsIndex[caseModel.itemId] = {
                            groupId: groupId,
                            item: caseModel,
                            assignedUserId: userId,
                        };
                    }
                });
                userToCasesList.push(userToCases);
                groupId += 1;
            });
            addDisbursementLinesLayer(disbursementLinesSource);
            self.setBusy(false);
        };

Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor comment.

@ajeety4
Copy link
Contributor Author

ajeety4 commented Dec 16, 2024

@ajeety4 Look good, but just one suggestion (feel free to evaluate and dispute it):

The handleDisbursementResults function iterates over all the users and cases, then the connectUserWithCasesOnMap iterates over all the users and cases again.

How about the following (note that I haven't tested it) - I removed the userToCasesList list to immediately construct the disbursementLinesSource (which would have been done in connectUserWithCasesOnMap), since we already have all the data we need to do so and we're skipping having to loop over all users/cases again:

        self.handleDisbursementResults = function (result) {
            // Clean stale disbursement results
            mapModel.removeDisbursementLayer();

            let groupId = 0;
            let disbursementLinesSource = [];
            Object.keys(result).forEach((userId) => {
                const user = mapModel.userMapItems().find((userModel) => {return userModel.itemId === userId;});
                mapModel.caseGroupsIndex[userId] = {groupId: groupId, item: user};

                let userToCases = {'user': user, 'cases': []};
                mapModel.caseMapItems().forEach((caseModel) => {
                    if (result[userId].includes(caseModel.itemId)) {
                        const lineCoordinates = [
                            [user.itemData.coordinates.lng, user.itemData.coordinates.lat],
                            [caseModel.itemData.coordinates.lng, caseModel.itemData.coordinates.lat],
                        ];
                        disbursementLinesSource.features.push(
                            {
                                type: 'Feature',
                                properties: {},
                                geometry: { type: 'LineString', coordinates: lineCoordinates },
                            }
                        );
                        
                        mapModel.caseGroupsIndex[caseModel.itemId] = {
                            groupId: groupId,
                            item: caseModel,
                            assignedUserId: userId,
                        };
                    }
                });
                userToCasesList.push(userToCases);
                groupId += 1;
            });
            addDisbursementLinesLayer(disbursementLinesSource);
            self.setBusy(false);
        };

Hey Charl , this is great point. The iteration is being done twice.
However the disbursement layer creation (and hence this source logic) is also called here from the review assignment . So in the favor of DRY and cleaner code, I think the iteration is fine in this case.

While looking through it, I did end up reusing the existing data structure and made some improvements. Changes are here

@ajeety4 ajeety4 requested a review from zandre-eng December 16, 2024 12:29
@ajeety4 ajeety4 merged commit 29e0b5e into master Dec 17, 2024
12 checks passed
@ajeety4 ajeety4 deleted the ay/optmise-disbursement-layer-creation branch December 17, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants