Posted by Zoltán Balogh on September 2, 2011 at 7:38am
6 followers
| Project: | Localization server |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
Issue Summary
We have an almost 2 years old issue: #589774: Missing original strings for some modules (was Missing module release data) While we can not find a solution for that issue, then I suggest a new feature. With a new, 'start over packages' permission the corresponding users are also able to start over the packages (without the 'administer localization server' permission). I have included the patch too.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| l10n_server.patch | 5.81 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in l10n_server_3.patch. | View details | Re-test |
Comments
#1
Ahh, not is_numeric($arg[3]). A better patch with is_numeric($arg[4]).
#2
Thanks for working on this! Quick feedback:
- !empty($arg[4]) should go before is_numeric() to avoid notices
- l10n_communinty_project_releases_table() adds a column at all times to the table but only adds a header to it if start_over is supposed to be there; the column is there anyway; let's not add the column!
- These messages are pretty pointless for the users targeted, because they cannot do these things: "If you clean up the database before this release is parsed again, source strings and translations will be lost. Always make database backups.", "Make sure to parse this release data again before deleting a project or cleaning up the database, or you will loose existing translation data.".
- Whether the release to be deleted belongs to the project linked is not checked; if I click a truncated link I can easily accidentally reset a release I did not want to; at least check if the release belongs to the project in question
#3
Thanks for the feedback, I corrected all of them.
#4
Patch tested and works well.
#5
+++ l10n_server/l10n_community/pages.inc 2011-09-02 11:05:33.000000000 +0200@@ -415,33 +415,93 @@
+function l10n_communinty_project_releases_table($releases, $project_uri, $project_name, $link_releases = TRUE, $start_over = FALSE) {
Where is this invoked with $start_over FALSE?
+++ l10n_server/l10n_community/pages.inc 2011-09-02 11:05:33.000000000 +0200@@ -415,33 +415,93 @@
+ if ($start_over) {
+ $rows[] = array(
+ $link_releases ? l($release_title, 'translate/projects/' . $project_uri . '/releases/' . $release->rid) : $release_title,
+ empty($release->download_link) ? t('n/a') : l(basename($release->download_link), $release->download_link),
+ format_date($release->file_date),
+ empty($release->last_parsed) ? t('in queue') : format_date($release->last_parsed),
+ $file_count,
+ $release->sid_count,
+ $error_count ? l($error_count, 'translate/projects/' . $project_uri . '/releases/' . $release->rid, array('fragment' => 'source-warnings')) : $error_count,
+ l(t('Start over'), 'translate/projects/' . $project_uri . '/releases/reset/' . $release->rid),
+ );
+ }
+ else {
+ $rows[] = array(
+ $link_releases ? l($release_title, 'translate/projects/' . $project_uri . '/releases/' . $release->rid) : $release_title,
+ empty($release->download_link) ? t('n/a') : l(basename($release->download_link), $release->download_link),
+ format_date($release->file_date),
+ empty($release->last_parsed) ? t('in queue') : format_date($release->last_parsed),
+ $file_count,
+ $release->sid_count,
+ $error_count ? l($error_count, 'translate/projects/' . $project_uri . '/releases/' . $release->rid, array('fragment' => 'source-warnings')) : $error_count,
+ );
...
+ $project = l10n_server_get_projects(array('uri' => $form_state['values']['project']->uri));
+ $releases = l10n_server_get_releases($form_state['values']['project']->uri, FALSE);
+ $release = $releases[$form_state['values']['release']->rid];
+
+ if (is_object($project) && is_object($release) && $project->pid == $release->pid) {
The only difference in these two arrays is the last item. Let's not duplicate code! Instead build-up a $row and tack on the last item when available. Then add to $rows.
+ $project = l10n_server_get_projects(array('uri' => $form_state['values']['project']->uri));+ $releases = l10n_server_get_releases($form_state['values']['project']->uri, FALSE);
+ $release = $releases[$form_state['values']['release']->rid];
+
+ if (is_object($project) && is_object($release) && $project->pid == $release->pid) {
Well, checking if $project->pid == $release->pid at this point looks like futile, since $release can only come from a list of releases from the project. Looks like we should check whether $releases[$form_state['values']['release']->rid] even exists, right? Same in l10n_communinty_projects_release_reset(). I can invoke that function with a URL of unrelated project/release info, and could get through.
#6
The l10n_communinty_project_releases_table() function is invoked with $start_over=FALSE from the l10n_community_project_release() (with $link_releases=FALSE too). Others are corrected.
#7
Works well. :)
#8
Committed to 6.x-3.x. Needs to be ported to 7.x-1.x
#9
Also rolled out to localize.drupal.org.
#10
Announced at http://localize.drupal.org/node/4124
#11
I am a translation community manager, but the Start over link is missing for me.
#12
Missing for me too. I assume I should have seen it on pages like http://localize.drupal.org/translate/projects/avitamin/releases or http://localize.drupal.org/translate/projects/avitamin/releases/46629 , right?
#13
Yes, on http://localize.drupal.org/translate/projects/avitamin/releases for example. My patch just creates a new permission, but I do not know, how Organic Groups works exactly. I think, nothing connects the "translation community manager" with the "start over".
#14
I've granted that permission when I rolled this out. This is how the permission matrix for our roles look like for the server: https://skitch.com/gabor.hojtsy/gy5qt/permissions-localize.drupal.org - the other granted permissions work. The og_user_roles module we have just inherits those settings for in-group permissions. I've also cleared the cache. Did that help any? (I can see those links: https://skitch.com/gabor.hojtsy/gy5xx/localized-drupal-distribution-rele... - although I'm clearly an admin here, that at least proves the update is rolled out proper).
#15
Now, I tested http://localize.drupal.org/translate/projects/avitamin/releases/reset/46629, and the access is denied. I (and nevergone) tested this patch without any organic groups modules. I readed the code again, the user_access('start over packages') is FALSE for the translation community managers, sure. What determines the display of the link?
<?phpfunction l10n_communinty_project_releases_table($releases, $project_uri, $project_name, $link_releases = TRUE, $start_over = FALSE) {
$rows = array();
$start_over = $start_over && user_access('start over packages');
?>
The $start_over parameter is fix TRUE, so the user_access('start over packages') must be FALSE. Otherwise the links are displayed. But, I don't know why not.
#16
All right, well, the group specific roles kick in when you *are* under a group. Looking at this, when you view this page it is not under any language group, so it will not grant you any permissions for that. Since you can be a member of any number of groups, possibly your permission for this would be the highest of your permissions from all the groups. That does sound like kind of overkill for this though and a pretty bad precedent... Looks like we need to figure out a way to have this UI somehow under a language group to keep the self-service nature of permission granting (vs. global permissions) and still have this feature work.
#17
I installed a 'full' l.d.o, with groups and with the used roles and permissions. The attached patch is adds the 'translation community manager' role (similarly, than og_user_roles) to the user account, if the user is outside from any group.
I tested, works well.
#18
updated: I tested the patch and not works well.
#19
start over permission problems
#20
Ahh, hook_user() is too late, then lets move this logic to hook_init().
#21
I tested the patch and very works well. :)
#22
This would widen the administer comments, administer nodes, etc. permissions these people have to the whole site, which is a no-go. The permissions of team managers should be confined to the group. As I've said above, this feature should be put under a group instead.
#23
Would not be easier a new role ("global permission") instead of "translation community manager", which has only the "start over packages" permission? This operation is not group-specific.
#24
I don't think its easier to explain what that would be to 100 always rotating language teams admins.... This is a huge crowd changing organically. Making the UI intuitive and in-place is best.
#25
Eg, I imagine you realize the strings are missing on the filter UI. So we can put on a little "troubleshooter" link there, like this: https://skitch.com/gabor.hojtsy/gb8nd/translate-localization-client-to-h... That page can have some explanation and a pointer to contact team admins if you don't have permission and a button if you do to start over the release? Seems like in-place in the group, much more closer to your workflow and can also be linked from the release list you worked with so far, since the page provides useful information even if you don't have permission to actually do something there. Also fits the permission/group model better.
#26
Ok, I working on it.
#27
First attempt (not RTBC), just for the discussion. What do you think?
#28
Looks like a good direction. (I think you can still keep the missing strings links in the release table too?)
#29
Yes, we can still the link in the releases table with "browse translations" permission, and the authenticated users can access that. But where to redirect that link? I need to search a translation group, where the user is a translation community manager?
#30
Well, right, ok, let's skip that then.
#31
I think, #27 will be good.
#32
#27: I tested the patch and works well.
#33
Thanks! Committed and pushed. Should deploy soon later today.
#34
Needs to be ported to D7.
#35
This should be deployed now, but it does not work. Using the project title instead of the short name and then pretending it is the short name does not really work. Also, it is displayed at a very odd place. I've suggested it would display under the release dropdown, right? Why are my suggestions ignored?
#36
BTW I've rolled back all patches committed from here, so you can start from a clean slate and broken functionality is not exposed on localize.drupal.org.
#37
I am sorry, but the real simulation of the LDO is an impossible mission in any other environment. For example, I dont know, how do you know the title of the projects? Which connector provides this information? Why does not works the og_user_roles module for me?
In my case the project titles and project uris are always the same, so the previous patch is works well as tested in this case. I prepared the final patch for the current 6.x-3.x-dev version, which is functionally equivalent, but there are two differences. Its using the l10n_community_project_uri_by_title function for the correct link creation, and not ignoring your suggestion about the place of the link. If it does not work, then I really give up, because my environment is different. I set the needs review status, but I think, it is superfluous, because the functionality is tested before, and nobody can test whether this will really work on the LDO.
#38
Thanks, committed and pushed, should be forward ported to Drupal 7 too.
#39
Ok, rolled out on localize.drupal.org too. Community managers have the start over permission enabled. Hope this works good now. Still to be ported.
#40
Ohh yeah, works well.
„Release data for Quova 7.x-1.0 was deleted from the Localization server.”
Thanks a lot.
#41
probably we have to add permission for a group of users for l.d.o with Start Over permission, not only admins
#42
All members of the "translation community manager" role have this permission.