Conversation
🚀 PR Environment Deployed
Access: https://era-13106.dev.pamdas.org |
luixlive
left a comment
luixlive
left a comment
There was a problem hiding this comment.
Translations seem fine. The main changes in PatrolMenu are minimal and elegant. No blocker. Just before approving, I have a couple things for you to think about.
1- Should we include a new button in our "Exports" section of the hamburger menu?
That would definitely increase the scope since we may need to add a modal, to select the patrol and the filter dates. Not sure if that makes sense.
2- The only condition we have now to show or not the new "Download" button is if the patrol has a leader, even if the leader has no tracks within the patrol bounds. This means that we can download GeoJSON files with no meaningful data. I'd suggest to improve the current condition to only allow downloading if there are tracks, what do you think?
luixlive
left a comment
luixlive
left a comment
There was a problem hiding this comment.
Approved 🎉
I still added a couple recommendations worth taking a look at. If they make sense and you have 5 minutes, go for them! But this is ready to be merged.
| // Track times are sorted newest-first; check range overlap using only the first/last entries | ||
| const trackNewest = new Date(patrolLeaderTrackTimes[0]).getTime(); | ||
| const trackOldest = new Date(patrolLeaderTrackTimes[patrolLeaderTrackTimes.length - 1]).getTime(); | ||
|
|
||
| return (!patrolStartTimestamp || trackNewest >= patrolStartTimestamp) | ||
| && (!patrolEndTimestamp || trackOldest <= patrolEndTimestamp); |
There was a problem hiding this comment.
The “tracks within patrol time range” check only compares the newest and oldest track timestamps. If the patrol has both a start_time and end_time, this can return true even when there are no track points inside the closed interval (e.g., all points are after end_time and/or before start_time but the overall span overlaps). Consider verifying at least one timestamp falls within [start_time, end_time] (binary search against the sorted times array would avoid scanning the whole array).
There was a problem hiding this comment.
Patrol tracks don't come from start/stop - This is okay
| import { DAS_HOST, PATROL_UI_STATES, PATROL_API_STATES } from '../constants'; | ||
| import { TRACKS_API_URL } from '../ducks/tracks'; | ||
| import { usePatrolsPermissions } from '../hooks/usePermissions'; |
There was a problem hiding this comment.
Importing TRACKS_API_URL from the tracks duck pulls in the entire ducks/tracks module (including reducer/actions and its dependency chain into utils/tracks, which imports the global store). Since this component only needs the URL builder, consider moving TRACKS_API_URL to a lightweight constants/api-urls module or constructing the URL from API_URL here to reduce coupling and avoid store side effects during module import (notably in tests).
There was a problem hiding this comment.
This is a common pattern through the project.
What does this PR do?
This pull request adds a new "Download Patrol Track" feature to the patrol options menu, allowing users to download the patrol track as a GeoJSON file. The feature is implemented with localization support across multiple languages and integrates with analytics tracking.
Feature: Download Patrol Track
KebabMenuin thePatrolMenucomponent, which allows users to download the patrol track as a.geojsonfile for the patrol leader. The download uses the patrol's serial number and leader's name for the filename, and includes analytics tracking. (src/PatrolMenu/index.js) [1] [2]DownloadArrowIcon,TRACKS_API_URL, anddownloadFileFromUrlutilities to support the download functionality. (src/PatrolMenu/index.js) [1] [2]Localization Updates
public/locales/en-US/patrols.json,public/locales/es/patrols.json,public/locales/fr/patrols.json,public/locales/ne-NP/patrols.json,public/locales/pt/patrols.json,public/locales/sw/patrols.json) [1] [2] [3] [4] [5] [6]Evidence
Relevant link(s)
Notes