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

Node and media delete action #1041

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

ajstanley
Copy link

What does this Pull Request do?

Provides an additional Action to delete a node along with all associated media and the files attached to those media.

What's new?

Provides the Action in code, and an instantiation of that action in config

This is new functionality and should not have any effect on existing code.

How should this be tested?

Once installed you should see a new action defined anywhere the Node Action Bulk Form is used (like the default content view)
image

Documentation Status

No existing behaviour is affected, and AFAIK we allow all actions to be self-documenting. It might be worth mentioning the 'no spaces in usernames if you want to delete Fedora files' somewhere

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/committers

@aOelschlager aOelschlager requested a review from DonRichards July 17, 2024 17:16
@DonRichards
Copy link
Member

I did test this last week on an existing isle-dc instance and it resulted in a white-screen. This might be a "user error" so I'm going to test again. I didn't test using the docs on Testing a Pull Request so I'll follow those instructions and see if the method I was using to testing the PR was the problem.

@DonRichards
Copy link
Member

Worked! Just adding notes on how I tested since there was an extra couple of steps not in the docs

composer config allow-plugins.cweagans/composer-patches true
composer require cweagans/composer-patches
curl -L https://github.com/Islandora/islandora/pull/1041.patch -o assets/pr-1041.patch
composer config extra.patches --merge --json '{"drupal/islandora": {"Node and media delete action: https://github.com/Islandora/islandora/pull/1041": "assets/pr-1041.patch"}}'
composer update drupal/islandora
chown -R nginx:  web/modules/contrib/islandora
drush config:export -y
cp web/modules/contrib/islandora/config/install/system.action.delete_node_and_media.yml config/sync/
chown -R nginx: config/sync/system.action.delete_node_and_media.yml
drush config:import -y
drush cr

DonRichards
DonRichards previously approved these changes Jul 24, 2024
@adam-vessey
Copy link

If the new config is absolutely required for the functionality, there should probably be an update or post-update hook implemented to add it, which would also avoid shenanigans with having to copy configs around, as you could instead just run the update hooks.

@DonRichards
Copy link
Member

Good call.

@DonRichards DonRichards dismissed their stale review July 25, 2024 18:09

A good point was brought up that an update hook is needed.

@ajstanley
Copy link
Author

Update hook added

@DonRichards
Copy link
Member

Testing this now.

@DonRichards
Copy link
Member

Steps taken

composer config allow-plugins.cweagans/composer-patches true
composer require cweagans/composer-patches
curl -L https://github.com/Islandora/islandora/pull/1041.patch -o assets/pr-1041.patch
composer config extra.patches --merge --json '{"drupal/islandora": {"Node and media delete action: https://github.com/Islandora/islandora/pull/1041": "assets/pr-1041.patch"}}'
composer update drupal/islandora
chown -R nginx:  web/modules/contrib/islandora
drush updatedb
drush cr

Copy link
Member

@DonRichards DonRichards left a comment

Choose a reason for hiding this comment

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

Worked as expected.

@DonRichards DonRichards merged commit c9d778e into Islandora:2.x Jul 30, 2024
27 checks passed
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.

3 participants