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.

Sign up for GitHub

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

Jump to bottom

Replace separators in strings to avoid confusion with export separator #3946

Merged
merged 9 commits into from Oct 2, 2023
Merged

Replace separators in strings to avoid confusion with export separator #3946

merged 9 commits into from Oct 2, 2023

Conversation

CarolineDenis
Copy link
Contributor

Fixes #3895

maxpatiiuk
maxpatiiuk requested changes Aug 26, 2023
View reviewed changes
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.

see #3946 (comment)

maxpatiiuk
maxpatiiuk approved these changes Aug 29, 2023
View reviewed changes
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.

looks good!
good job refactoring the code to match the needs

@CarolineDenis CarolineDenis marked this pull request as ready for review August 29, 2023 14:16
@CarolineDenis CarolineDenis requested a review from a team August 29, 2023 14:16
@CarolineDenis CarolineDenis modified the milestones: 7.9.2, 7.9.1 Aug 29, 2023
@CarolineDenis CarolineDenis linked an issue Aug 29, 2023 that may be closed by this pull request
Records with commas break Selected Create CSV #3895
Closed
carlosmbe
carlosmbe previously requested changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

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

Slay. It works great with formatted Agents Queries. However, it's not working well with formatted Locality Queries.

Screenshot 2023-09-25 at 3 32 05 PM Screenshot 2023-09-25 at 3 32 17 PM

@CarolineDenis
Copy link
Contributor Author

Slay. It works great with formatted Agents Queries. However, it's not working well with formatted Locality Queries.

Screenshot 2023-09-25 at 3 32 05 PM Screenshot 2023-09-25 at 3 32 17 PM
Screenshot 2023-09-26 at 8 44 40 AM
@carlosmbe, the data starting in column 2 are phantom fields that are being added to the export file by mistake. I know it's confusing. There's already an open issue for that. From your screenshot, we can see that the formatted column hasn't been split where there is a comma, which is the expected behavior here.

@CarolineDenis CarolineDenis requested a review from a team September 27, 2023 13:12
Base automatically changed from v7.9-dev to production September 28, 2023 01:25
bronwyncombs
bronwyncombs approved these changes Oct 2, 2023
View reviewed changes
Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

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

Formatted fields aren't broken up and appear in CSV as they do in query builder results.

Screenshot 2023-10-02 at 9 29 53 AM
Screenshot 2023-10-02 at 9 35 29 AM

@CarolineDenis CarolineDenis dismissed carlosmbe’s stale review October 2, 2023 16:25

fixed in other issue

@CarolineDenis CarolineDenis merged commit 10ac4ed into production Oct 2, 2023
9 checks passed
@CarolineDenis CarolineDenis deleted the issue-3895 branch October 2, 2023 16:25
@maxpatiiuk
Copy link
Member

maxpatiiuk commented Oct 3, 2023 •
edited

@CarolineDenis (sanity check) I might be missing context but why do some columns on this screenshot don't have column headings?
and why are some columns completely blank?

image

If those are the phantom fields, they should be excluded from the CSV export

@CarolineDenis
Copy link
Contributor Author

@CarolineDenis (sanity check) I might be missing context but why do some columns on this screenshot don't have column headings? and why are some columns completely blank?

image

If those are the phantom fields, they should be excluded from the CSV export

Yes you are right, it's the phantom. an other issue is open for that. Vinny is working on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxpatiiuk maxpatiiuk maxpatiiuk approved these changes

@bronwyncombs bronwyncombs bronwyncombs approved these changes

@melton-jason melton-jason Awaiting requested review from melton-jason

@carlosmbe carlosmbe Awaiting requested review from carlosmbe

Assignees
No one assigned
Labels
None yet
Projects
General Tester Board
Status: ✅Done
Milestone
7.9.1
Development

Successfully merging this pull request may close these issues.

Records with commas break Selected Create CSV
5 participants
@CarolineDenis @maxpatiiuk @carlosmbe @bronwyncombs @melton-jason