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

Use Mapbox Layer for Cases and User Markers in Microplanning Map #35535

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

zandre-eng
Copy link
Contributor

@zandre-eng zandre-eng commented Dec 18, 2024

Product Description

The markers themselves will now look slightly different as shown below:

Old:
Screenshot from 2024-12-18 12-16-08

New:
Screenshot from 2024-12-18 12-15-17

Technical Summary

Link to ticket here.
Related tech spec doc can be found here.

This PR introduces a core change to the Microplanning Map page in the form of completely changing how markers are loaded onto the map. Previously, the markers were created as separate HTML elements which were then added onto the map, however this caused performance issues if there were a lot of markers on the map (e.g. 1000) and ultimately limited the scalability of this feature.

The marker logic has been reworked to use a single Mapbox data source with two layers, one for users and one for cases. The markers themselves are shown on the map with a simple image which gets loaded in when initializing the map. Using a map layer instead of individual HTML elements for the markers allows the page to achieve greater performance which, in turn, will allow for displaying more markers on the map at any given time.

In addition to the above implementation, this PR also resolves the following two bugs on the Microplanning Map page:

  • Reviewing assignment results with polygon filtering applied resulted in a JS exception being thrown in the browser.
  • Running a new disbursement does not remove the previous disbursement results from the assignment popup.

Feature Flag

MICROPLANNING

Safety Assurance

Safety story

  • Local testing
  • QA to be done

End-to-end testing was done locally for both the Microplanning Map and Case Clustering Map pages to ensure that all functionality still works as expected. Additionally, QA will be doing end-to-end testing on these two pages as well to make sure all features still work.

Automated test coverage

N/A

QA Plan

QA to be completed. Ticket can be viewed here.

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

@zandre-eng zandre-eng added awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled labels Dec 18, 2024
@zandre-eng
Copy link
Contributor Author

Keeping this PR in draft until the related Jira ticket to determine what size to increase the pagination to has been implemented in this PR as well. QA will also be held off until that has been completed.

@zandre-eng zandre-eng marked this pull request as ready for review December 20, 2024 07:35
Copy link
Contributor

@ajeety4 ajeety4 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Nice refactors along the way. Left a couple of minor comments.

Just wanted to confirm that as we removed the old markers logic in this PR, does the Case Clustering map works correctly (specifically the Lock Case Grouping functionality) ?

That map also uses the html markers currently and will eventually be replaced to layers as well as part of its own upcoming scaling effort.

@zandre-eng
Copy link
Contributor Author

Just wanted to confirm that as we removed the old markers logic in this PR, does the Case Clustering map works correctly (specifically the Lock Case Grouping functionality) ?

@ajeety4 I have done local testing on this page to ensure all functionality still works as expected. Additionally, QA is also taking a pass at this page to do end-to-end testing of all the features on this page.

It should be noted that the Case Clustering page has its own models for the markers and such, and so is not affected by the removal of the ones from models.js. It's a bit strange that we had this duplication of similar models, however this should get cleaned up when we eventually get to replacing the markers on this page as well.

@ajeety4
Copy link
Contributor

ajeety4 commented Jan 7, 2025

@ajeety4 I have done local testing on this page to ensure all functionality still works as expected. Additionally, QA is also taking a pass at this page to do end-to-end testing of all the features on this page.

It should be noted that the Case Clustering page has its own models for the markers and such, and so is not affected by the removal of the ones from models.js. It's a bit strange that we had this duplication of similar models, however this should get cleaned up when we eventually get to replacing the markers on this page as well.

Got it. Thanks for confirming on this.

@orangejenny
Copy link
Contributor

@zandre-eng When merging master, I got a lint failure email and realized that the linter can't parse case_grouping_map.js due to the usage of async. It looks like this has been the case for a while, but it would be good to fix so we have visibility into other issues (after removing the async, the file has about 70 other errors/warnings). Could you take a look at making that file lint-able? ESLint has a couple of options around function styles and parsing options that might help.

@zandre-eng
Copy link
Contributor Author

When merging master, I got a lint failure email and realized that the linter can't parse case_grouping_map.js due to the usage of async. It looks like this has been the case for a while, but it would be good to fix so we have visibility into other issues (after removing the async, the file has about 70 other errors/warnings). Could you take a look at making that file lint-able? ESLint has a couple of options around function styles and parsing options that might help.

@orangejenny It seems the big culprit here is the eslint ecmaVersion we are currently using (v6), as this version doesn't support declaring async functions. Since we only declare a JS async function in this one place in the codebase, I've created a separate PR here which refactors the problem function to use promise chaining instead. This effectively resolves the linting issue.

@orangejenny
Copy link
Contributor

@zandre-eng Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants