Filters reset after approving suggestion
helmo - October 9, 2009 - 10:45
| Project: | Localization server |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | claudiu.cristea |
| Status: | closed |
Description
After approving a set of translations in the moderate tab on localize.drupal.org, the filters are reset.
I had for example filtered on one specific project, after submitting the entries from the first page all other projects appeared again.
But this also happens when you fiter on a text string.
In my opinion these filters should be preserved.
#1
subscribe
#2
Attached is patch that should fix that.
Please test & review...
#3
I just installed 6.x-1.x-dev on my dev machine and applying the patch seems to have the desired effect.
#4
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:
<?php// 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:
<?php
/**
* 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.
#5
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.
#6
OK. Fixed all the above stuff...
#7
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.
#8
Automatically closed -- issue fixed for 2 weeks with no activity.