We need a warning to the user before disassociating all content from Lingotek.

CommentFileSizeAuthor
#36 interdiff-32-33.txt857 bytesmdahl328151
#36 2884160-disassociate-confirm-33.patch2.91 KBmdahl328151
#34 Selection_105.png187.25 KBmdahl328151
#34 2884160-disassociate-confirm-32.patch2.92 KBmdahl328151
#34 interdiff-31-32.txt836 bytesmdahl328151
#32 2884160-disassociate-confirm-31.patch1.99 KBmdahl328151
#32 2884160-disassociate-confirm-31-THIS-SHOULD-FAIL.patch1.26 KBmdahl328151
#29 2884160-disassociate-confirm-29.patch11.78 KBpenyaskito
#29 2884160-disassociate-confirm-28-29.interdiff.txt1.68 KBpenyaskito
#28 2884160-disassociate-confirm-28.patch11.65 KBpenyaskito
#28 2884160-disassociate-confirm-27-28.interdiff.txt9.57 KBpenyaskito
#27 2884160-disassociate-confirm-27.patch10.88 KBpenyaskito
#25 interdiff-9-15.txt3.64 KBmdahl328151
#25 warning_before_disassociate-2884160-15.patch11.21 KBmdahl328151
#24 interdiff-9-14.txt5.05 KBmdahl328151
#24 warning_before_disassociate-2884160-14.patch6.22 KBmdahl328151
#22 interdiff-9-13.txt4.52 KBmdahl328151
#22 warning_before_disassociate-2884160-13.patch11.14 KBmdahl328151
#18 interdiff.txt4.51 KBmdahl328151
#18 warning_before_disassociate-2884160-12.patch11.13 KBmdahl328151
#16 interdiff.txt4.51 KBmdahl328151
#16 warning_before_disassociate-2884160-11.patch17.38 KBmdahl328151
#14 warning_before_disassociate-2884160-11.patch6.24 KBmdahl328151
#11 warning_before_disassociate-2884160-9.patch11.41 KBmdahl328151
#8 warning_before_disassociate-2884160-6.patch11.07 KBmdahl328151
#5 warning_before_disassociate-2884160-3.patch6.08 KBmdahl328151
#2 warning_before_disassociate-2884160-2.patch10.66 KBmdahl328151
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdahl328151 created an issue. See original summary.

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-2.patch10.66 KB
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Active » Needs review

I guess the proper status is Needs review. We would need tests for this (hopefully they exist, and we only need to update them)

Status: Needs review » Needs work

The last submitted patch, 2: warning_before_disassociate-2884160-2.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-3.patch6.08 KB
mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: warning_before_disassociate-2884160-3.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-6.patch11.07 KB
mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: warning_before_disassociate-2884160-6.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-9.patch11.41 KB
mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented 
  1. +++ b/src/Form/LingotekSettingsTabDisassociateForm.php
@@ -0,0 +1,149 @@
+/**
+ * Configure Lingotek
+ */

    We need to update the class docs. 

  2. +++ b/lingotek.routing.yml
@@ -111,6 +111,14 @@ lingotek.import:
+lingotek.confirm_disassociate:
+  path: '/admin/lingotek/confirm_disassociate'

    I have to check if this is the best way of doing this. I wonder what happens if I go directly to this path. 

  3. +++ b/src/Form/LingotekSettingsTabDisassociateForm.php
@@ -0,0 +1,149 @@
+/**
+ * Configure Lingotek
+ */

    We need to update this docs. 

  4. +++ b/src/Form/LingotekSettingsTabDisassociateForm.php
@@ -0,0 +1,149 @@
+  protected $profile;
+  protected $profile_vaults;
+  protected $auto_upload_disabled;
+  protected $auto_download_disabled;
+  protected $profile_name_disabled;
+  protected $profile_index;
+  protected $profile_usage;
+  protected $is_custom_id = 1; // Disallow removing for now.

    What is this for?
    I think lots of class properties here are unused? 

  5. +++ b/src/Form/LingotekSettingsTabDisassociateForm.php
@@ -0,0 +1,149 @@
+  public function Disassociate(array &$form, FormStateInterface $form_state) {

    lowercase 

  6. +++ b/src/Form/LingotekSettingsTabDisassociateForm.php
@@ -0,0 +1,149 @@
+    /** @var \Drupal\lingotek\LingotekConfigTranslationServiceInterface $translation_service */
+    $translation_service = \Drupal::service('lingotek.config_translation');

    Let's inject this on the class. 

  7. +++ b/src/Form/LingotekSettingsTabDisassociateForm.php
@@ -0,0 +1,149 @@
+    /** @var \Drupal\lingotek\LingotekContentTranslationServiceInterface $translation_service */
+    $translation_service = \Drupal::service('lingotek.content_translation');

    Let's inject this on the class.

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-11.patch6.24 KB

Patch didn't include the disassociated form.

Status: Needs review » Needs work

The last submitted patch, 14: warning_before_disassociate-2884160-11.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review
FileSize
warning_before_disassociate-2884160-11.patch17.38 KB
interdiff.txt4.51 KB

Status: Needs review » Needs work

The last submitted patch, 16: warning_before_disassociate-2884160-11.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-12.patch11.13 KB
interdiff.txt4.51 KB
mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: warning_before_disassociate-2884160-12.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review
FileSize
warning_before_disassociate-2884160-13.patch11.14 KB
interdiff-9-13.txt4.52 KB

Status: Needs review » Needs work

The last submitted patch, 22: warning_before_disassociate-2884160-13.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review
FileSize
warning_before_disassociate-2884160-14.patch6.22 KB
interdiff-9-14.txt5.05 KB
mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
warning_before_disassociate-2884160-15.patch11.21 KB
interdiff-9-15.txt3.64 KB

The last submitted patch, 24: warning_before_disassociate-2884160-14.patch, failed testing. View results

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
FileSize
2884160-disassociate-confirm-27.patch10.88 KB

Rerolled.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
FileSize
2884160-disassociate-confirm-27-28.interdiff.txt9.57 KB
2884160-disassociate-confirm-28.patch11.65 KB

Made some changes:

* lingotek.confirm_disassociate should be a form, not a controller. We skip one redirect call with that.
* Remove LingotekSettingsController::disassociate() then, as it is not needed.
* Renamed LingotekSettingsTabDisassociateForm to LingotekDisassociateAllConfirmForm and made it extend from ConfirmFormBase
* Expanded testing in LingotekUtilitiesDisassociateAllDocumentsTest.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Needs review » Reviewed & tested by the community
FileSize
2884160-disassociate-confirm-28-29.interdiff.txt1.68 KB
2884160-disassociate-confirm-29.patch11.78 KB

Code standards and fixed undefined variable in exception report.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Reviewed & tested by the community » Fixed

Committed 03c548c and pushed to 8.x-2.x. Thanks!

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Fixed » Needs review
FileSize
2884160-disassociate-confirm-31-THIS-SHOULD-FAIL.patch1.26 KB
2884160-disassociate-confirm-31.patch1.99 KB

Minor change. This test simulates content with missing metadata

The last submitted patch, 32: 2884160-disassociate-confirm-31-THIS-SHOULD-FAIL.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
FileSize
interdiff-31-32.txt836 bytes
2884160-disassociate-confirm-32.patch2.92 KB
Selection_105.png187.25 KB

Another minor change to avoid this display:

Status: Needs review » Needs work

The last submitted patch, 34: 2884160-disassociate-confirm-32.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review
FileSize
2884160-disassociate-confirm-33.patch2.91 KB
interdiff-32-33.txt857 bytes

Status: Needs review » Needs work

The last submitted patch, 36: 2884160-disassociate-confirm-33.patch, failed testing. View results

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented
Status: Needs work » Needs review
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Title: Warning before disassociate all content » Require confirmation before disassociating all content
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented 
+++ b/src/LingotekContentTranslationService.php
@@ -789,7 +789,7 @@ class LingotekContentTranslationService implements LingotekContentTranslationSer
-    if ($metadata) {
+    if ($metadata && $metadata->getContentEntityTypeId() && $metadata->getContentEntityId()) {

I'm guessing this leaves all those metadata still around and never got removed, right?

mdahl328151’s picture

mdahl328151 CreditAttribution: mdahl328151 commented 
+++ b/src/LingotekContentTranslationService.php
@@ -789,7 +789,7 @@ class LingotekContentTranslationService implements LingotekContentTranslationSer
+    if ($metadata && $metadata->getContentEntityTypeId() && $metadata->getContentEntityId()) {

Yes. This is for the metatdata that is still in the database, but that is missing a content entity type id or a content entity id
I guess this is a work around. The real solution is to completely get rid of metadata if there is no content assigned to it, right?

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Needs review » Reviewed & tested by the community

Right, I'm going to commit this as it is. See #2899711: "Disassociate All" could be faster just by deleting all metadata objects for a follow up that we may want to explore.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Reviewed & tested by the community » Fixed

Committed 4e3fa42 and pushed to 8.x-2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.