Filters reset after approving suggestion

subscribe

Attached is patch that should fix that.

Please test & review...

I found that the call to l10n_community_build_filter_values() is redundant while filters were obtained previously by the same method. Removed that call.

Also optimizing a bit. I found that this piece of code is used now in 3 places:

    // Replace some values by their string representation.
    foreach (array('project' => 'uri', 'author' => 'name') as $name => $key) {
      if (!empty($filters[$name])) {
        $filters[$name] = $filters[$name]->$key;
      }
    }

... so I replaced it with a new function in translate.inc:

/**
 * Helper function used to replace complex data filters (like objects or arrays)
 * with their string representation.
 *
 * @param array $filters
 *   Associative array with filters passed. This array is passed by reference and will be altered here.
 */
function l10n_community_flat_filters(&$filters) {
  static $replacements = array('project' => 'uri', 'author' => 'name');
  
  foreach ($replacements as $name => $key) {
    if (!empty($filters[$name])) {
      $filters[$name] = $filters[$name]->$key;
    }
  }
}

Here's the new patch.

Overall, looks good!

Detailed feedback:

+++ l10n_community/moderate.inc	16 Oct 2009 09:14:30 -0000
@@ -52,6 +52,12 @@ function l10n_community_moderation_form(
+
+  // Send filters directly to the submit callback.
+  $form['filters'] = array(
+    '#type' => 'value',
+    '#value' => $filters,
+  );
@@ -126,6 +132,13 @@ function l10n_community_moderation_form_
+
+  $filters = $form_state['values']['filters'];
+  // Replace some values by their string representation for URL redirects.
+  l10n_community_flat_filters($filters);
+  
+  // Redirect keeping the relevant filters intact in the URL.
+  $form_state['redirect'] = array($_GET['q'], $filters);

What the translate.inc code does is to set $form['#redirect'] early on, so why not use the same pattern? That seems to require less coding and logic, especially none in the submission callback.

+++ l10n_community/translate.inc	16 Oct 2009 09:14:30 -0000
@@ -268,11 +264,7 @@ function l10n_community_translate_form(&
-    }
-  }
+    l10n_community_flat_filters($filters);
 
   $form = array(

Too many spaces used for indentation.

+++ l10n_community/translate.inc	16 Oct 2009 09:14:30 -0000
@@ -767,3 +759,20 @@ function l10n_community_build_filter_val
+
+/**
+ * Helper function used to replace complex data filters (like objects or arrays)
+ * with their string representation.
+ *
+ * @param array $filters
+ *   Associative array with filters passed. This array is passed by reference and will be altered here.
+ */
+function l10n_community_flat_filters(&$filters) {
+  static $replacements = array('project' => 'uri', 'author' => 'name');
+  
+  foreach ($replacements as $name => $key) {
+    if (!empty($filters[$name])) {
+      $filters[$name] = $filters[$name]->$key;
+    }
+  }
+}

Very good that this abstraction is there. It was already at multiple places.

Please use one line function introduction comments though to conform to the standard.

Also, we do not provide the type name in Drupal phpdoc, that is how it is, so remove "array".

This review is powered by Dreditor.

OK. Fixed all the above stuff...

Ok, I worked with this code a bit. The flattened filter arrays are really only needed for the redirection, so only used them there. Also shortened some code comments. Committed this one. Thanks for all your help.