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

Add kml selection exports #5300

Open
wants to merge 13 commits into
base: production
Choose a base branch
from
Open

Add kml selection exports #5300

wants to merge 13 commits into from

Conversation

alesan99
Copy link
Contributor

@alesan99 alesan99 commented Oct 2, 2024

Fixes #3589

Adds the ability to export a KML file from a query using only the selected query results.

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

  • In query builder, create a locality query with coordinate data (like Latitude1 and Longitude1).
  • Run the query and "Create KML"
  • Make sure the KML was exported correctly. The result should be the exact same as one created on production.
  • Select any rows of the query results.
  • Make sure the KML includes ONLY the rows that were selected. (Also keep in mind that any rows without valid coordinate data will not be exported.)
  • Make sure CSV exports still work.

Fix type errors (TODO: use XmlNode)
Fix data format
Add xml header
Attempt to fix types
@realVinayak
Copy link
Contributor

While I'm sure that this is done to mimic CSV on frontend, I think a better implementation would be just to make an API call, but this time adding an extra filter to just select the ids that the user chose. Benefits:

  1. Much simpler to implement
  2. If in future we find a bug in KML on backend, we don't need to waste time fixing on frontend.
  3. No need to reimplement the wheel
  4. Most importantly (/s), I'll need to be doing something similar for batch-edit BUT I plan on making it a backnd way (snding filtrs). So, if you implement it right, I can reuse it! If you want, you can use wb_improvements to implement it straight-away (I'm not sure where it lies in priority, so ask @CarolineDenis too).

@maxpatiiuk thoughts? I think, even for CSV, it would have been better to directly use backend.

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

Good use of the utility functions that we have available!

Nevertheless, I agree with Vinny that if it would reduce duplication, we should do this work on the back-end.


Aside from that, better communication about who is working on what is always nice to avoid redundant effort

@alesan99
Copy link
Contributor Author

While I'm sure that this is done to mimic CSV on frontend, I think a better implementation would be just to make an API call, but this time adding an extra filter to just select the ids that the user chose. Benefits:

I agree. I already came across issues myself having to implement the same feature twice on my last CSV export PR.

I think the only downside is we'd lose the instant downloads from the frontend exports.
But I'd like to take a look into it and see if I'm able to do that.

@realVinayak
Copy link
Contributor

I don't think it necessarily matters, especially for KML exports. For CSV, it is quite handy to do it within a few clicks, so don't change that (or users wouldn't like extra clicks needed now). For KML, definitely go the backend route. You'd learn about the query builder API quite a bit in the process!

@alesan99 alesan99 marked this pull request as ready for review October 28, 2024 18:30
@alesan99 alesan99 requested review from maxpatiiuk and a team October 28, 2024 18:31
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

👍

@alesan99 alesan99 requested a review from a team October 29, 2024 13:29
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • In query builder, create a locality query with coordinate data (like Latitude1 and Longitude1).
  • Run the query and "Create KML"
  • Make sure the KML was exported correctly. The result should be the exact same as one created on production.
  • Select any rows of the query results.
  • Make sure the KML includes ONLY the rows that were selected. (Also keep in mind that any rows without valid coordinate data will not be exported.)
  • Make sure CSV exports still work.

If you try to create KML without selecting any rows nothing happens. Creating a KML and selecting rows does work along with creating a CSV with or without selecting rows.

@alesan99 alesan99 requested review from emenslin and a team November 4, 2024 18:00
@alesan99 alesan99 added this to the 7.9.8 milestone Nov 4, 2024
Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

  • In query builder, create a locality query with coordinate data (like Latitude1 and Longitude1).
  • Run the query and "Create KML"
  • Make sure the KML was exported correctly. The result should be the exact same as one created on production.
  • Select any rows of the query results.
  • Make sure the KML includes ONLY the rows that were selected. (Also keep in mind that any rows without valid coordinate data will not be exported.)
  • Make sure CSV exports still work.

Looks great! Everything works as expected.

@pashiav pashiav requested a review from a team November 4, 2024 19:10
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • In query builder, create a locality query with coordinate data (like Latitude1 and Longitude1).
  • Run the query and "Create KML"
  • Make sure the KML was exported correctly. The result should be the exact same as one created on production.
  • Select any rows of the query results.
  • Make sure the KML includes ONLY the rows that were selected. (Also keep in mind that any rows without valid coordinate data will not be exported.)
  • Make sure CSV exports still work.

Looks good!!

@alesan99 alesan99 requested review from a team and removed request for a team November 5, 2024 15:50
@CarolineDenis CarolineDenis modified the milestones: 7.9.8, 7.9.9 Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Dev Attention Needed
Development

Successfully merging this pull request may close these issues.

Query Builder export to KML ignores selected rows
6 participants