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 civiimport core extension with Import api code for viewing import tables #24230

Merged
merged 2 commits into from
Aug 18, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 11, 2022

Overview

Add civiimport core extension with Import api code for viewing import tables.

This PR is primary about adding new pseudo-crud api for interacting with the data import tables. The data import tables are short-lived (default is a week, although we do not yet have a good cleanup process) tables that hold the data the user is trying to import plus some status & informational fields. The goal of adding an API is to get logic out of quickforms & associated classes & standardise it and to get the ability to leveerage search kit, afform etc.

With this PR each import UserJob is not passed will be visible as an api pseudoentity (similar to eck) and will support crud actions per below. Note that the fields available are based on the field names in the user's data source - so in the example the user had a column 'Amount Given' (or similar).

Because of the above editable search kit displays can be added - although I have opted to keep the commit that adds them for a follow up PR to keep this focussed on api rather than ui.

image

The intent of putting the code in an extension is that it feels like the import code should be separated into a core extension as a code management consideration - but it is not intended to be an optional extension (maybe one day in the far far future). I need to work through the steps of making it always enabled & hidden and that tests run on it - but those are not blocking on merging this & generally I find a few steps are required AFTER merging any new extension.

I also have the intention to follow up with some additional apis / actions available to search-kit - most obviously
validate and import which will allow a user to edit-in-place a row that failed & re-try it. However, I'm also expecting some metadata functions to facilitate getting more logic out of the forms & potentially replacing some chunks of forms with angular (the relevant forms are quite modular)

Before

No api support for interacting with Import pseudo-entities, not available in search kit

After

   $importFields = Import::getFields($userJobID)->execute();

    Import::create($userJobID)->setValues([
      'external_identifier' => 678,
      'amount_given' => 80,
      '_status' => 'NEW',
    ])->execute();

  $import = Import::get($userJobID)->setSelect(['external_identifier', 'amount_given', '_status'])->execute()->first();
   

    Import::update($userJobID)->setValues([
      'amount_given' => NULL,
      '_id' => $import['_id'],
      '_status' => 'IMPORTED',
    ])->execute();


    Import::save($userJobID)->setRecords([
      [
        'external_identifier' => 999,
        'amount_given' => 9,
        '_status' => 'ERROR',
      ],
    ])->execute();

Technical Details

We do a couple of checks before exposing an Import to the api

  • the table can be determined and exists
  • the expires_date is not in the past - in future we will have a cleanup routine to get rid of tables with a past expires_date so it will be equivalent to the first condition

Permissioning is based on checking if the UserJob in question will load for the relevant user - https://github.com/civicrm/civicrm-core/pull/24230/files#diff-4077b8d652237c281456106d3f92c6e6a1cc3ec3f8dd3da117b58fc102c94d37R89-R98

Comments

I spun off a couple of PRs #24285 with the changes that are outside the extension #24275

@civibot
Copy link

civibot bot commented Aug 11, 2022

(Standard links)

@civibot civibot bot added the master label Aug 11, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import_api branch 3 times, most recently from 843b63b to b7421e2 Compare August 11, 2022 22:34
Comment on lines 72 to 88
'title' => ts('Import') . '_' . $userJobID,
'title_plural' => ts('Import') . $userJobID,
Copy link
Member

Choose a reason for hiding this comment

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

This is kinda ugly and not the right way to use ts(), is there no user-facing label for these temp tables?

'type' => ['Import'],
'table_name' => $tableName,
'class_args' => [$userJobID],
'label_field' => '_id',
Copy link
Member

Choose a reason for hiding this comment

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

Guess there's no label field? That could be a problem, how is the user supposed to choose the correct import job entity in SearchKit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I could append the user display name - ie import2-by-Coleman Watts

@eileenmcnaughton eileenmcnaughton force-pushed the import_api branch 15 times, most recently from d201da4 to 028902e Compare August 16, 2022 12:05
@eileenmcnaughton eileenmcnaughton changed the title [WIP] Add civiimport with Import api code for viewing import tables Add civiimport with Import api code for viewing import tables Aug 16, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import_api branch 2 times, most recently from 2be4c6f to 4176929 Compare August 16, 2022 23:33
@eileenmcnaughton eileenmcnaughton changed the title Add civiimport with Import api code for viewing import tables Add civiimport core extension with Import api code for viewing import tables Aug 17, 2022
@colemanw
Copy link
Member

colemanw commented Aug 17, 2022

@eileenmcnaughton - doesn't have to be in this PR but maybe the label could be combination of {entity} {date} {user} like "Contact Import 8/15/2022 by Coleman Watts"...

Alternatively, (maybe better) we could add a column for _label and populate it with the name of the file.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw hmm ok - I'll ponder those ideas - I am inclined to do it as a follow up (if this passes)

@colemanw
Copy link
Member

@eileenmcnaughton there's more overiding than necessary going on in the APIv4 classes. Ideally they shouldn't need to do anything special. #24290 ought to make that possible.

@eileenmcnaughton eileenmcnaughton force-pushed the import_api branch 2 times, most recently from 50579ae to facfb8e Compare August 17, 2022 03:06
@eileenmcnaughton
Copy link
Contributor Author

CiviApiImportTest::testApiActions
Error: Call to undefined method Civi\Api4\Import::create()

/home/jenkins/bknix-dfl/build/core-24230-7aju4/web/sites/all/modules/civicrm/ext/civiimport/tests/phpunit/CiviApiImportTest.php:76
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:1721

@colemanw
Copy link
Member

@eileenmcnaughton I thought create wasn't allowed? Isn't this an update-only entity?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw yeah - I don't see a downside to having a CREATE api but not a current need (other than to create in the test) either. We wouldn't want to expose create to afform though

@colemanw
Copy link
Member

Ok. I'll add it back. I think we need a way to tell afform what actions are allowed per entity, because I'm also working on CiviCase for Afform and that only supports create not update.

@eileenmcnaughton
Copy link
Contributor Author

don't we mark some 'read-only' for that reason?

@eileenmcnaughton
Copy link
Contributor Author

Hmm nope

CiviApiImportTest::testApiActions
CRM_Core_Exception: Import records can only be updated not created; "_id" is required.

@colemanw
Copy link
Member

don't we mark some 'read-only' for that reason?

"Read-only" impies no create AND no update. We want to allow one not the other.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw it passed

@colemanw colemanw merged commit 027e0b1 into civicrm:master Aug 18, 2022
@colemanw colemanw deleted the import_api branch August 18, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants