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

[Import] Output errors, duplicates csvs directly from the user job table #23292

Merged
merged 6 commits into from
May 5, 2022
Merged

[Import] Output errors, duplicates csvs directly from the user job table #23292

merged 6 commits into from
May 5, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 24, 2022 •
edited
Loading

Overview

[REF] [Import] Output errors, duplicates csvs directly from the user job table

Before

During parsing the errors and duplicate-skips are output to a csv file

After

The csvs are generated on demand from the import temp table using a url like

/civicrm/import/outcome?user_job_id=5&status=4&reset=1

Note that this means

  1. only the user that did the import can access the output file
  2. the file can be re-accessed until the job is cleaned up - at this stage the UI doesn't offer a way to re-visit it but the url can be re-visited

Technical Details

Handling of import outcomes was inconsistent - the download mechanism for the rows that were not updating the csv is the next task

Most commits in this PR are also in other open PRs - only the last 1 is new to this PR - requires a menu./rebuild

Comments

I found the summary screen confusing - total rows was total rows less those that were already rejected in the Preview screen - that seemed confusing to me so it now reflects the total number of rows - I think it might still be a little odd until I get everything updating in the table

@civibot
Copy link

civibot bot commented Apr 24, 2022

(Standard links)

@civibot civibot bot added the master label Apr 24, 2022
@eileenmcnaughton eileenmcnaughton changed the title Import output table [REF] [Import] Output errors, duplicates csvs directly from the user job table Apr 24, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import_output_table branch 2 times, most recently from f0aa451 to d3a4f1e Compare April 24, 2022 04:54
@eileenmcnaughton eileenmcnaughton changed the title [REF] [Import] Output errors, duplicates csvs directly from the user job table [Import] Output errors, duplicates csvs directly from the user job table Apr 24, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import_output_table branch 3 times, most recently from ede5be3 to 0c49c3d Compare April 26, 2022 19:41
@eileenmcnaughton eileenmcnaughton mentioned this pull request Apr 26, 2022
Merged
This was referenced Apr 30, 2022
Merged
Closed
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 3, 2022
@eileenmcnaughton
Add extra functions to DataSource
dc9add7 
This adds functions from civicrm#23292 to
the DataSource class (without them being called as yet)

- part of trying to get to the point where fixes are not dependent on each other
@eileenmcnaughton eileenmcnaughton mentioned this pull request May 3, 2022
Merged
@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor Author

test this please

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 3, 2022
@eileenmcnaughton
Add extra functions to DataSource
7c195e0 
This adds functions from civicrm#23292 to
the DataSource class (without them being called as yet)

- part of trying to get to the point where fixes are not dependent on each other
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 3, 2022
@eileenmcnaughton
Add extra functions to DataSource
59c4a2c 
This adds functions from civicrm#23292 to
the DataSource class (without them being called as yet)

- part of trying to get to the point where fixes are not dependent on each other
@eileenmcnaughton
Copy link
Contributor Author

unrelated fail

image

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 3, 2022
@eileenmcnaughton
Add extra functions to DataSource
a518d6f 
This adds functions from civicrm#23292 to
the DataSource class (without them being called as yet)

- part of trying to get to the point where fixes are not dependent on each other
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 3, 2022
@eileenmcnaughton
Add extra functions to DataSource
46a7c32 
This adds functions from civicrm#23292 to
the DataSource class (without them being called as yet)

- part of trying to get to the point where fixes are not dependent on each other
@eileenmcnaughton eileenmcnaughton force-pushed the import_output_table branch 5 times, most recently from 0050f62 to f1a3155 Compare May 4, 2022 23:25
@eileenmcnaughton
Copy link
Contributor Author

It only failed on the known error but test this please cos I'm going for the green tick

@eileenmcnaughton
[REF] [Import] Remove always NULL parameter from private function
c968e73
@eileenmcnaughton
Cleanup cleanup on old tables for form re-submission
60912c7
@eileenmcnaughton
Move adding status fields on import table to datasource
8d88fae
@eileenmcnaughton
Make tablename a property of the datasource
3377d52 
The datasource stores the table name on the job - we don't need to pass it around.

Also - stop cleaning up the temp table at the end - we want it to
output results but will add a cleanup routine later
@eileenmcnaughton
Add output form for csv-on-the-fly
99e3c5f
@eileenmcnaughton
Remove conflict download from preview
d3bb0df 
I can't see where these would arise - but definitely
not on the preview screen - which only runs basic validation
@monishdeb
Copy link
Member

monishdeb commented May 5, 2022 •
edited
Loading

  • General standards
    • (r-explain) PASS
    • (r-user) PASS: This is an security improvement and offers a secure way to download import errors in CSV using a secure link.
    • (r-doc) PASS : I think PR description explains in detail
    • (r-run) PASS: I have tested the patch in the test-build generated http://core-23292-1xd8l.test-1.civicrm.org:8001 ( admin/ vKtkNnwzABaX ) Here's a screencast, the preview screen now provide secure link to download the import errors, on demand:
      before1

The error link here can be revisited directly as logged in user but would be nice to have an option in UI to revisit the unfinished import.

  • Developer standards
    • (r-tech) PASS
    • (r-code) PASS : Liked the way we are generating the import errors on demand, also solves a security issue where earlier it puts the CSV file in an accessible public directory.
    • (r-maint) PASS
    • (r-test) PASS

@monishdeb monishdeb merged commit a863d40 into civicrm:master May 5, 2022
@colemanw colemanw deleted the import_output_table branch May 5, 2022 18:20
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb thanks! - I totally agree re re-visiting - defintely something I hope to get to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eileenmcnaughton @monishdeb