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 FormBuilder forms to Civiimport #25072

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Add FormBuilder forms to Civiimport

Before

Without FormBuilder wrappers we can't embed the forms

After

Now we can...

Technical Details

Comments

@colemanw - these are created if Civiimport is enabled when importing

@civibot
Copy link

civibot bot commented Nov 29, 2022

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

}
catch (UnauthorizedException $e) {
// No access - return the empty array.
}
Copy link
Member

Choose a reason for hiding this comment

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

@eileenmcnaughton unless I'm missing something, all this can be simplified. You don't need the per-user caching & you don't need the permission check when fetching SearchDisplays and you don't need the try/catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colemanw yeah - you do want all that - the user will only retrieve their own jobs by the acls - so it is per user. And if they don't have permission to the api at all then we want to catch the error

Copy link
Member

Choose a reason for hiding this comment

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

Ok so is there some hidden mechanism checking permissions inside the SearchDisplay::get() that only returns certain searches per-user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - cos it applies ACLs & the ACLS on the UserJob filter

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I've addressed your feedback (and confirmed the docs step-by-step guide on how to embed a form is inadequate.... https://docs.civicrm.org/dev/en/latest/afform/overview/#embedding )

Co-authored-by: colemanw <coleman@civicrm.org>
@colemanw
Copy link
Member

@eileenmcnaughton I've expanded the docs to be slightly more adequate. The code snippets are untested so will need to be adjusted as we go along.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw - I'll see if I can make sense of it - this will be less painful to work on if we put 'My imports' in navigation - do you think under 'Reports' - it's the best I can think of

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Nov 30, 2022
@colemanw
Copy link
Member

Ok @eileenmcnaughton I'm fine with you merging this whenever you're ready

@eileenmcnaughton
Copy link
Contributor Author

@colemanw yeah - I'm ready on this one - the other is slated as a follow up (embedding)

@eileenmcnaughton
Copy link
Contributor Author

@colemanw OK - having sume success with - 301c8de

@eileenmcnaughton
Copy link
Contributor Author

Upgraded to Merge-on-pass based on above comment & because I don't intend to add more to this PR

@eileenmcnaughton eileenmcnaughton merged commit 330e63b into civicrm:master Nov 30, 2022
@eileenmcnaughton eileenmcnaughton deleted the import_form branch November 30, 2022 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge on pass merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants