Description for Delete option is confusing

Created on 20 December 2023, about 1 year ago
Updated 21 May 2024, 7 months ago

Problem/Motivation

I found myself a little confused by the "Delete redirects defined in the spreadsheet" option, especially the "This action can be undone" description. However, also the option itself had me thinking that it would remove existing redirects and replace with those in the file (not sure how, user intuition isn't always fully rational). I think the idea is that the redirects will only be removed when they exactly match the definition in the spreadsheet. The most common use case would seem to be to undo a previous import. That also means the deletion can be undone by importing the same file again, which is probably what is behind the extra description "This action can be undone". Is this correct?

Proposed resolution

I think this can be clarified by rewording things a little.

Option: Undo previous import
Description: This deletes redirects in the file when they match exactly.

This makes it more obvious that this is (mostly?) meant as an undo option. The restore action also becomes implicitly and obviously clear: re-import the file again.

Remaining tasks

  • Confirm the functionality and its intention
  • Agree whether this is a good idea
  • Make the change
  • Review
  • Merge

User interface changes

Wording of the "Delete redirects" option is changed, as outlined above.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Needs work

Version

2.0

Component

User interface

Created by

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @eelkeblok
  • 🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
  • Assigned to sourabhjain
  • 🇮🇳India sourabhjain

    Let me work on this.

  • Issue was unassigned.
  • Status changed to Needs work about 1 year ago
  • 🇮🇳India sourabhjain

    I have updated the changes but when I am trying to test the feature before and after the changes, I am getting below error in logs.

    The file temporary://path_redirect_import/export_1703158296.csv was not deleted because it does not exist.
    

    Can someone help me on it.
    Attaching the updated patch.

  • 🇩🇪Germany Anybody Porta Westfalica

    I agree the current description is a bit unclear and might be misleading. But before implementing any changes, someone please has to check the code, what it exactly does and then describe that in detail.

    I agree that in many cases you may want to overwrite potentially existing redirects, let's tackle this with a separate option in Add option to overwrite existing source path redirects Active

    I think delete option may also make sense to other cases to keep.

  • 🇩🇪Germany Anybody Porta Westfalica

    This is the method that returns the redirect entities to be deleted when using the option:

      /**
       * Iterates over the CSV file to load the list of redirects to delete.
       *
       * @return \Drupal\redirect\Entity\Redirect[]
       *   Array containing the Redirect entities to delete.
       *
       * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
       * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
       * @throws \Drupal\migrate\MigrateException
       * @throws \League\Csv\Exception
       */
      protected function redirectsToDelete() {
        $redirects_to_delete = [];
        $storage = $this->entityTypeManager->getStorage('redirect');
        $header = $this->getReader()->getHeader();
        $records = $this->getReader()->getRecords($header);
    
        foreach ($records as $record) {
          $parsed_url = UrlHelper::parse(urldecode($record['source']));
          $path = $parsed_url['path'] ?? NULL;
          $query = $parsed_url['query'] ?? NULL;
          $hash = Redirect::generateHash($path, $query, $record['language']);
    
          // Search for redirects by hash.
          $redirects = $storage->loadByProperties(['hash' => $hash]);
          $redirects_to_delete = array_merge($redirects_to_delete, $redirects);
        }
        return $redirects_to_delete;
      }
    
Production build 0.71.5 2024