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

Fix/batch upload children, with validation according to default widget #896

Merged
merged 22 commits into from
Nov 7, 2022

Conversation

adam-vessey
Copy link

GitHub Issue: Islandora/documentation#2129

  • Other Relevant Links (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

What does this Pull Request do?

Implements a multi-(/two-)step wizard for bulk child node (and media) creation, where:

  • The first step identifies the type of media, allowing the target media field to be isolated; and,
  • The second step renders the default widget for the field (with the cardinality unlocked to allow an arbitrary number of items), permitting the upload proper.

... By using the widget this way, ideally we maintain any relevant validation regarding file types and sizes and the like, but also should make use of the field's configured upload location and more.

What's new?

Implemented the wizard(s) and deprecated the old forms.

  • Does this change add any new dependencies? ctools is now a dependency
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? No.
  • Could this change impact execution of existing code? Any alterations of the old forms may need to be updated.

How should this be tested?

Really, should be effectively regression-testing the two batch upload mechanisms being replaced with multi-step wizards:

Use the child-addition wizard to "Batch Upload Children" to create child nodes of a given node. /node/{node}/members/upload, should be present as a local action on the /node/{node}/members route... its use should be relatively straight-forward?

Use the media-addition wizard to "Batch Upload Media" to create media on a given node. /node/{node}/media/upload, should be present as a local action on the /node/{node}/media route... and should be similarly straight-forward to use.

Documentation Status

  • Does this change existing behaviour that's currently documented? Possibly?
  • Does this change require new pages or sections of documentation? Probably not?
  • Who does this need to be documented for?
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/8-x-committers

adam-vessey and others added 15 commits July 28, 2022 11:33
... before the colon is the project name, so should only be "drupal" for
modules shipped in core.
... bit of refactoring stuff making a mess.
... as in, basically functional. Still needs coding standards pass, and
testing with more/all types of content.
... 'cause need to slap together a media-specific batch similarly?
Both the child-uploading, and media-uploading forms.
... no longer necessarily trying to load files, where files might not
be present (for non-file media... oEmbed things?).
... is defined instead at the "FileSelectionForm" level, accidentally
left it here from intermediate implementation state.
@jordandukart
Copy link
Member

Couple general comments for discussion (not necessarily related to the implementation / code):

  • Should the existing upload forms be removed or deprecated? I'd personally lean towards removal but @adam-vessey has left them in for the time being given that it's not clear if other people could have extended/implemented them.
  • The implementation within this PR mirrors the existing functionality to provide parity but the original does have some oddities. An example would be allowing any content type (regardless of the presence of a model field) to be added to a collection.

@rosiel rosiel self-assigned this Aug 24, 2022
@rosiel
Copy link
Member

rosiel commented Sep 13, 2022

I'm testing this and at http://localhost:8000/node/1/members/upload I get "The requested page could not be found."

@adam-vessey
Copy link
Author

@rosiel: Could be a couple of things:

  • if composer wasn't used to manage the dependency (adding a repository entry pointing at my fork, composer require'ing it, which should grab ctools and whatever it in turn might require), ctools may not be present... any errors about missing classes in your watchdog logs?
  • could just need to clear cache, as the class handling the route has changed?

@wgilling
Copy link

bump -- let's talk about this soon again if no movement by the next I8 Tech call

@rosiel
Copy link
Member

rosiel commented Sep 28, 2022

Some progress.

  1. I had used Composer to install your version, and ctools was downloaded by Composer, but it was not enabled in Drupal. I don't know how to mitigate this programmatically.
  2. However even with ctools enabled, I'm getting WSOD "The website encountered an unexpected error. Please try again later" at http://localhost:8000/node/1/members/upload. At least this time there are better errors in the logs. I have run drush cr and the Rebuild Menu command from the Devel toolbar.

TypeError: Argument 6 passed to Drupal\ctools\Wizard\FormWizardBase::__construct() must implement interface Drupal\Core\Render\RendererInterface, string given, called in /var/www/html/drupal/web/modules/contrib/islandora/src/Form/AddChildrenWizard/AbstractForm.php on line 65 in Drupal\ctools\Wizard\FormWizardBase->__construct() (line 100 of /var/www/html/drupal/web/modules/contrib/ctools/src/Wizard/FormWizardBase.php)

#0 /var/www/html/drupal/web/modules/contrib/islandora/src/Form/AddChildrenWizard/AbstractForm.php(65): Drupal\ctools\Wizard\FormWizardBase->__construct(Object(Drupal\Core\TempStore\SharedTempStoreFactory), Object(Drupal\Core\Form\FormBuilder), Object(Drupal\Core\DependencyInjection\ClassResolver), Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher), Object(Drupal\Core\Routing\RouteMatch), 'islandora.uploa...', NULL, 'type_selection')
#\1 [internal function]: Drupal\islandora\Form\AddChildrenWizard\AbstractForm->__construct(Object(Drupal\Core\TempStore\SharedTempStoreFactory), Object(Drupal\Core\Form\FormBuilder), Object(Drupal\Core\DependencyInjection\ClassResolver), Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher), Object(Drupal\Core\Routing\RouteMatch), 'islandora.uploa...', Object(Drupal\Core\Session\AccountProxy), NULL, 'type_selection')

@adam-vessey adam-vessey marked this pull request as draft October 3, 2022 15:18
@adam-vessey
Copy link
Author

adam-vessey commented Oct 3, 2022

Ah right, forgot about that whole issue, of the .info.yml not being used at update time to enable new dependencies... somewhat of an odd thing to search for, as well... https://www.drupal.org/node/3067480 seems to suggest that the new dependency should be enabled in an update in the module (in the "after" bit):

allow installed modules services to depend on yet uninstalled module services as long as the existing module installs the new module in an update hook

Can make it so...

... as for the other bit... was doubting my sanity at first, looking at it, but, looking back, looks like things were dev'd against ctools 3.7... and the introduction of the $renderer parameter only came in 3.8: https://git.drupalcode.org/project/ctools/-/commit/78788f48d9ea421ed41be1c040ec7c4ebf9a5d09 ... so yeah ... fun. Use of ReflectionFactory-like things (https://git.drupalcode.org/project/ctools/-/blob/516d85fe04073f87ebd35f900ae9ba882c21f505/src/Wizard/WizardFactory.php#L86-97) seems to run counter to extensibility, as if you have to introduce additional properties to be set in the constructor, you would pretty much have to go trigger all the reflection stuff against the parent class to allow it to pick up its new parameters... blurf... but whatever, probably unnecessary here. Can just bump the spec up to ^3.8, and add in the new parameter.

@adam-vessey adam-vessey marked this pull request as ready for review October 3, 2022 20:08
@adam-vessey
Copy link
Author

I think this should be good now, @rosiel.

@seth-shaw-asu
Copy link
Member

Tech Call: question about ctools version 3 || 4?

Gave it a run through here, and seemed to work fine; however, ctools'
project page still seems to suggest the 3 major version should be
preferred... but let's allow 4, if people are using or want to test it
out?
@adam-vessey
Copy link
Author

adam-vessey commented Oct 5, 2022

Gave it a test with ctools 4(.0.1, the version that a composer require "drupal/ctools:^4" gave me, at present), ran a drush cache:rebuild and the stuff here seems to work, so went ahead and bumped the spec to allow it in addition to the ^3.8, due to ctools' suggestion to use the 3 version:

The 3.x branch is the most current version of ctools, designed for Drupal 9 and 10. Once Drupal 10 is released, this module will be compatible with the LTS version of Drupal 9.
The 4.x branch is experimental and should not be used. New features around context plugins will go into the 4.1.x branch, which will supercede 3.x when it gets released.
Dependent modules should rely on the 3.x module.

@rosiel
Copy link
Member

rosiel commented Oct 10, 2022

I updated to your latest commit, and the (children) wizard worked. I made three islandora_object children (node 2, 3, and 4, with medias 1, 2, and 3). However, this was my log message list right after.

12 log errors thrown at the same time

The three 'search api' errors i'm not worried about, but some of the others seem like they might be asking for things before they're in place. Here's more detail on the islandora error. A yellow flag went up when i saw it was appending "?_format=jsonld" to the batch url:

Screen Shot 2022-10-10 at 9 12 23 AM

@adam-vessey
Copy link
Author

adam-vessey commented Oct 11, 2022

@rosiel : Of that, yeah the one "undefined index count" thing, is a goof on my part, fix'd; however, the other bits... not really sure:

  • Warning: hash_file( [...] bit
    • happens on one-off media ingests connected to nodes, so pre-existing/separate issue?
      • Not sure if "connected to nodes" is particularly relevant there... suspecting it could happen on any particular file/media uploads.
  • Error generating event: Bundle islandora_object does not exist
    • could not reproduce on a fresh islandora-playbook instance, based on starter_dev... so not sure?
  • page not found with ?_format stuff
    • Not sure what to say, about this. Trying to compare against the previous version of these endpoints, to see if things are similarly thrown; however, the previous endpoints are completely non-functional as per the related ticket... so it's not possible to see if this was introduced here... that said, it does sound vaguely familiar, like it was talked about at one of the tech calls in the last couple of months... somewhat similar to Don't emit JMS event on file upload until the associated Media is saved documentation#2113 , perhaps? Like, milliner is trying to access things before they're available, or something?... any which way, these endpoints are more functional with this PR, in that they allow for the files to be uploaded and media to be created, than without and not being able to upload the files at all.
    • Seems like this can pop also on one-off media uploads (as in, doing the single-media-under-a-node thing) so... yeah, looking like a separate issue

@rosiel
Copy link
Member

rosiel commented Nov 3, 2022

@wgilling i've done another run through and the fixes @adam-vessey fixed are, indeed fixed. I'm 100% ok with getting this merged but wanted to check if you had any reservations, since you're the requested reviewer.

@rosiel rosiel merged commit 3f7ca2c into Islandora:2.x Nov 7, 2022
rosiel pushed a commit to rosiel/islandora that referenced this pull request Nov 7, 2022
Islandora#896)

* Add ctools, prior to using it.

* Fix up all the dependency references.

... before the colon is the project name, so should only be "drupal" for
modules shipped in core.

* Some more together.

* Decent progress... getting things actually rendering...

... bit of refactoring stuff making a mess.

* More worky.

... as in, basically functional. Still needs coding standards pass, and
testing with more/all types of content.

* Coding standards, and warning of validation issues.

* Pull the batch out to a separate service.

* Something of namespacing the child-specific batch...

... 'cause need to slap together a media-specific batch similarly?

* All together, I think...

Both the child-uploading, and media-uploading forms.

* It is not necessary to explicitly mark the files as permanent.

* Further generalizing...

... no longer necessarily trying to load files, where files might not
be present (for non-file media... oEmbed things?).

* Adjust class comment.

* Get rid of the deprecation flags.

* Remove unused constant.

... is defined instead at the "FileSelectionForm" level, accidentally
left it here from intermediate implementation state.

* Pass the renderer along, with the version constraint.

* Add update hook to enable ctools in sites where it may not be.

... as it's now required.

* Cover ALL the exits.

* Refine message.

* Excessively long line in comment...

... whoops.

* Bump spec up to allow ctools 4.

Gave it a run through here, and seemed to work fine; however, ctools'
project page still seems to suggest the 3 major version should be
preferred... but let's allow 4, if people are using or want to test it
out?

* Fix undefined "count" index.
@wgilling
Copy link

wgilling commented Nov 9, 2022

hrough and the fixes

I am ok - and the separate question of ctools version has not been addressed in our Born Digital ops sprints as of yet.

rosiel pushed a commit to rosiel/islandora that referenced this pull request Dec 14, 2022
Islandora#896)

* Add ctools, prior to using it.

* Fix up all the dependency references.

... before the colon is the project name, so should only be "drupal" for
modules shipped in core.

* Some more together.

* Decent progress... getting things actually rendering...

... bit of refactoring stuff making a mess.

* More worky.

... as in, basically functional. Still needs coding standards pass, and
testing with more/all types of content.

* Coding standards, and warning of validation issues.

* Pull the batch out to a separate service.

* Something of namespacing the child-specific batch...

... 'cause need to slap together a media-specific batch similarly?

* All together, I think...

Both the child-uploading, and media-uploading forms.

* It is not necessary to explicitly mark the files as permanent.

* Further generalizing...

... no longer necessarily trying to load files, where files might not
be present (for non-file media... oEmbed things?).

* Adjust class comment.

* Get rid of the deprecation flags.

* Remove unused constant.

... is defined instead at the "FileSelectionForm" level, accidentally
left it here from intermediate implementation state.

* Pass the renderer along, with the version constraint.

* Add update hook to enable ctools in sites where it may not be.

... as it's now required.

* Cover ALL the exits.

* Refine message.

* Excessively long line in comment...

... whoops.

* Bump spec up to allow ctools 4.

Gave it a run through here, and seemed to work fine; however, ctools'
project page still seems to suggest the 3 major version should be
preferred... but let's allow 4, if people are using or want to test it
out?

* Fix undefined "count" index.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants