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 - start to use civicrm_user_job to track user imports #23245

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Import - start to use civicrm_user_job to track user imports #23245

merged 3 commits into from
Apr 21, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 19, 2022 •
edited
Loading

Overview

Import - start to use civicrm_user_job to track user imports.

While this is primarily a code change to support further cleanups & addition of functionality it does fix one issue - the loss of the submitted values in the data source section of the form when going forwards and back - with this PR the following works

  1. Fill in the DataSource form with sql type and add a query - click next
  2. click back - the query is still present

Note that this also works for csv with 'skipColumnHeader' - annoyingly it doesn't for the file upload field - the defaults are set but I don't quite know how defaults work with file upload fields - maybe @colemanw or @demeritcowboy does?

Before

civicrm_user_job has been added to the schema but we have not started to use it

After

When submitting the first form (DataSource) in the Import contact flow a record is added to the civicrm_user_job table holding

  • submitted_values- these will be added to by those submitted on other forms in the flow - notably the mapping - so the job could be picked up & run later & also so we don't have to do a weird dance to pass them aroun
  • DataSource - values relating to the datasource - so far table_name - the created table and column_headers - the relevant headers

Technical Details

Next steps include updating the parser class to use the userJobID to get the data it needs rather than pass in a huge array on 'run' - ideally it would take $userJobID in the construct() and should not accept any other values. Lots more cleanup to come

Comments

Merging #23253 will improve readability

@civibot
Copy link

civibot bot commented Apr 19, 2022

(Standard links)

@civibot civibot bot added the master label Apr 19, 2022
@eileenmcnaughton eileenmcnaughton changed the title Import wip Import wip - am going clean this up shortly - many of the commit are already open in PRs Apr 19, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import_wip branch 5 times, most recently from 35a76e5 to 411e220 Compare April 19, 2022 21:16
@eileenmcnaughton eileenmcnaughton changed the title Import wip - am going clean this up shortly - many of the commit are already open in PRs Import - start to use user_job to track user imports Apr 19, 2022
@eileenmcnaughton eileenmcnaughton changed the title Import - start to use user_job to track user imports Import - start to use civicrm_user_job to track user imports Apr 19, 2022
@demeritcowboy
Copy link
Contributor

demeritcowboy commented Apr 19, 2022 •
edited
Loading

how defaults work with file upload fields

They don't since that would be a security issue - code could default it to upload /etc/passwd and such. It's a browser restriction not a civi restriction.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy ok thanks - good to know

@eileenmcnaughton eileenmcnaughton force-pushed the import_wip branch 5 times, most recently from 149f88e to 9d284cd Compare April 20, 2022 00:35
@totten
Copy link
Member

totten commented Apr 20, 2022

civibot, test this please

@eileenmcnaughton eileenmcnaughton force-pushed the import_wip branch 2 times, most recently from 9d57ab5 to 6ce886e Compare April 20, 2022 01:01
@eileenmcnaughton eileenmcnaughton mentioned this pull request Apr 20, 2022
Merged
@eileenmcnaughton
[REF] [Import] Strict typing on function that throws an exception if …
c256233 
…null
@eileenmcnaughton
Initial permission on UserJob
e36f7ee 
This adds
1) an acl so only the creator can access and
2) permits anyone with access CiviCRM to GET their
own jobs
3) permission on CREATE is set to administer civicrm

I am thinking that create might be too strict but
it might be better to start this way. Likewise we
probably want sysadmins to be able to access other
people's jobs but unless we have a plan now for
what permissions we want we can punt the question
by keeping these strict for now
@eileenmcnaughton
Start to use user_job to track import
7b057b6
@eileenmcnaughton
Copy link
Contributor Author

#23260 extends this PR to make some improvements to the MapField form & template & Preview - it might be easier to try all together - all focussed on Contact import flow

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

Console log is unclear ...

I found

INFO: Processing JUnit
INFO: [JUnit] - 1 test report file(s) were found with the pattern 'junit/karma.xml' relative to '/home/jenkins/workspace/CiviCRM-Core-PR' for the testing framework 'JUnit'.
WARNING: The file '/home/jenkins/workspace/CiviCRM-Core-PR/junit/karma.xml' is an invalid file.
WARNING: At line 3 of file:/home/jenkins/workspace/CiviCRM-Core-PR/junit/karma.xml:cvc-complex-type.3.2.2: Attribute 'assertions' is not allowed to appear in element 'testsuite'.
ERROR: Step ‘Publish xUnit test result report’ failed: The result file '/home/jenkins/workspace/CiviCRM-Core-PR/junit/karma.xml' for the metric 'JUnit' is not valid. The result file has been skipped.
Setting status of 7b057b6 to FAILURE with url

@eileenmcnaughton eileenmcnaughton mentioned this pull request Apr 20, 2022
Merged
@eileenmcnaughton
Copy link
Contributor Author

This is only fails on an infra related issue - but might as well get a clean slate - test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor Author

This is only fails on an infra related issue - but might as well get a clean slate - test this please

@eileenmcnaughton
Copy link
Contributor Author

unrelated test fails - however, the tests have gotten messed up now on the ones that build on this during the test issues over the last bit. @colemanw & I had previous discussed & @colemanw blessed the PR that builds off the PR that builds off this PR to merge. However, now that one has a problem - so I'm going to merge this based on the authorization of the follow up so I can start to unravel what is going on.

Note that the mitigating factor in all this is that more PRs are coming with more r-run to do on them over the same bit of code.

@eileenmcnaughton eileenmcnaughton merged commit 90976f4 into civicrm:master Apr 21, 2022
@eileenmcnaughton eileenmcnaughton deleted the import_wip branch April 21, 2022 05:20
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.
3 participants
@eileenmcnaughton @demeritcowboy @totten