Problem/Motivation

As part of the l10n_update integration in core the motivation of this issue is to provide the user interface which displays the translation status and allows users to perform translation updates in certain languages.

Proposed resolution

The goal is to provide an overview of the available interface translation updates on a per-language level. Also to give detailed information for administrators about projects which can be updated. Already up-to-date projects are not shown anywhere, however for debugging purposes projects which updates were not found for are shown (development or unknown releases). A message also appears on this screen to inform administrators about when updates have been checked last time, along with a link for checking manually.

Remaining tasks

  • Build the UI according to the proposal in #50.
  • Shorten summary texts. See 2-3. points in Bojhan's comment.
  • Add whitespace between the check manually message and the table.
  • Add tests to cover this functionality.
  • Add Translation status to the Status report page.
  • Use real module/theme name instead of machine name
  • Add feature to show/hide low priority columns on mobile device, as per module page
  • UI review (see #63)
  • Accessibility concerns (see #64 and #86)
  • code review
  • update status message screenshots
  • update issue summary in general
  • open remaining follow-up issues based on answers to questions from #89

How to work on this issue

To test this issue:

  • Apply the latest display-interface-translation-status-1804702-*.patch from this issue queue.
  • Enable Locale module under Extend
  • Add one or more languages (Dutch) under Configuration
  • Check the Available translation status by clicking the link The report, Available translation updates which leads to admin/reports/translations. This will show updates not found for drupal 8.x
  • Check the Status report at admin/reports/status. This will show Updates not found too.
  • Optionally: Install Locale Tamper module. This will mock some 7.x modules for which translations will be downloaded.
  • revisit the available translation status admin/reports/translations and "check manually".
  • expand to see more detail
  • Check the Status report at admin/reports/status. This will show Updates and link to updates report page.

User interface changes

New administrative page at /admin/reports/translations.
review-2012-12-13_1446.png
Status summary on Status report page at /admin/reports/status.
Translation status Ok
Updates available
no updates found 2012-12-13_1436.png
No status info available

Follow-ups

Need to be opened

  • #89 step 6, Q2. 3 of 3 when checking manually.

Unrelated Issues opened

Original report by Sutharsan

As part of the l10n_update integration in core this issue adds the function to display the translation status and provide the user interface . See the UML in #1191488: META: Integrate l10n_update functionality in core for the big picture where this issue fits in.

Parts provided by this issue:

  • A status page with translations to be updated. The interface was previously discussed at #1029554: Translations update feature user experience.
  • A button to update the translations on the above page.
  • A message on the status page that translation are available.
  • A link to manually check for updates

Other related functions:

  • (Download and )import translations when a module is enabled
  • (Download and )import translations when a language is added
  • Update the translation when a module is updated
  • Update interface translations automatically on cron
CommentFileSizeAuthor
#106 display-interface-translation-status-1804702-106.patch31.91 KBSutharsan
#106 interdiff-1804702-103-106.txt4.62 KBSutharsan
#105 display-interface-translation-status-1804702-103.patch31.79 KBSutharsan
#105 interdiff-1804702-101-103.txt1.88 KBSutharsan
#101 display-interface-translation-status-1804702-101.patch31.79 KBSutharsan
#101 interdiff-1804702-96-101.txt8.73 KBSutharsan
#97 display-interface-translation-status-1804702-96.patch32.46 KBYesCT
#97 interdiff-86-96.txt6.85 KBYesCT
#96 review-2012-12-13_1446.png126.28 KBYesCT
#96 no updates found 2012-12-13_1436.png18.15 KBYesCT
#89 di-86-s04-2012-12-12_0224.png56.35 KBYesCT
#89 di-86-s04a-2012-12-12_0232.png44.16 KBYesCT
#89 di-86-s07-status_report-2012-12-12_0243.png74.44 KBYesCT
#89 di-86-s08-updates_collapsed-2012-12-12_0309.png70.08 KBYesCT
#89 di-86-s09-2012-12-12_0319.png112.84 KBYesCT
#89 di-86-s10-status_updates available_warning-2012-12-12_0405.png47.83 KBYesCT
#89 di-86-s11-2012-12-12_0420.png82.76 KBYesCT
#89 di-86-s12-expanded-2012-12-12_0423.png108.5 KBYesCT
#86 display-interface-translation-status-1804702-86.patch31.27 KBSutharsan
#86 interdiff-1804702-75-86.patch1.71 KBSutharsan
#79 di-why-2012-12-08_0512.png61.92 KBYesCT
#79 di-8of8-2012-12-08_0512.png75.7 KBYesCT
#78 di-t01-status-no-2012-12-08_0447.png113.66 KBYesCT
#78 di-t02-2012-12-08_0451.png23.04 KBYesCT
#78 di-t03-added_spanish-2012-12-08_0453.png107.34 KBYesCT
#78 di-t04-first_core-2012-12-08_0456.png71.72 KBYesCT
#77 di-u01-2012-12-08_0439.png131.3 KBYesCT
#76 di-s01-8-2012-12-08_0036.png104.23 KBYesCT
#76 di-s02-8-report-2012-12-08_0037.png45.84 KBYesCT
#76 di-s03-8-manual-2012-12-08_0038.png58.14 KBYesCT
#76 di-s04-report-2012-12-08_0054.png61.19 KBYesCT
#76 di-s05-expanded-2012-12-08_0056.png62.52 KBYesCT
#76 di-s06-manual-2012-12-08_0057.png66.08 KBYesCT
#76 di-s07-admin_status_report-2012-12-08_0308.png109.23 KBYesCT
#76 di-s08-tamper-check_manually-2012-12-08_0320.png72.72 KBYesCT
#76 di-s09-tamper-expanded-2012-12-08_0320.png109.68 KBYesCT
#76 di-s10-tamper-admin_status_report-2012-12-08_0322.png83.01 KBYesCT
#76 di-s11-after_updates-plural-2012-12-08_0325.png104.68 KBYesCT
#76 di-s12-expanded-2012-12-08_0326.png95.66 KBYesCT
#75 display-interface-translation-status-1804702-75.patch31.22 KBYesCT
#75 interdiff-67-75.txt2.85 KBYesCT
#69 display-interface-translation-status-1804702-67.patch30.99 KBGábor Hojtsy
#68 display-interface-translation-status-1804702-67-do-not-test.patch30.99 KBSutharsan
#67 display-interface-translation-status-1804702-67.patch222.05 KBSutharsan
#67 display-interface-translation-status-1804702-67-do-not-test.patch1.1 MBSutharsan
#67 interdiff-1804702-60-67.txt18.13 KBSutharsan
#60 display-interface-translation-status-1804702-60-do-not-test.patch19.86 KBSutharsan
#60 interdiff-1804702-59-60.txt20.62 KBSutharsan
#60 locale-translation-status-page-60.png114.02 KBSutharsan
#59 locale-translation-status-page-59.png96.43 KBSutharsan
#59 interdiff-1804702-45-59.txt13.18 KBSutharsan
#59 display-interface-translation-status-1804702-59-do-not-test.patch13.5 KBSutharsan
#50 updates-i18n-shortdescription.png69.7 KBBojhan
#48 locale-translation-status-page-47.png39.28 KBSutharsan
#46 interdiff-1804702-45-46.txt3.26 KBSutharsan
#46 display-interface-translation-status-1804702-46-do-not-test.patch19.26 KBSutharsan
#46 locale-translation-status-page.png59.21 KBSutharsan
#46 locale-translation-status-available.png32.95 KBSutharsan
#46 locale-translation-status-not-found.png33.04 KBSutharsan
#46 locale-translation-status-ok.png12.72 KBSutharsan
#46 locale-translation-status-unknown.png33.03 KBSutharsan
#45 interdiff-1804702-34-45.txt12.87 KBSutharsan
#42 ModulesPage.jpg33.41 KBGábor Hojtsy
#37 display-interface-translation-status-1804702-37-do-not-test.patch12.15 KBbalintk
#37 interdiff-1804702-34-37.txt13.5 KBbalintk
#37 display-interface-translation-status-1804702-37-1-small.png27.28 KBbalintk
#37 display-interface-translation-status-1804702-37-1.png67.37 KBbalintk
#37 display-interface-translation-status-1804702-37-2-small.png40.53 KBbalintk
#37 display-interface-translation-status-1804702-37-2.png112.32 KBbalintk
#34 display-interface-translation-status-1804702-34.patch8.37 KBSutharsan
#33 display-interface-translation-status-1804702-33.patch8.4 KBSutharsan
#32 display-interface-translation-status-1804702-31.png60.38 KBSutharsan
#31 display-interface-translation-status-1804702-31.patch7.65 KBSutharsan
#30 display-interface-translation-status-1804702-30-do-not-test.patch8.43 KBbalintk
#30 interdiff-1804702-20-30.txt6.92 KBbalintk
#30 Screenshot94.22 KBbalintk
#30 Screenshot thumbnail70.51 KBbalintk
#24 available-translation-updates-large.png463.84 KBbalintk
#20 available-translation-updates.png80.86 KBbalintk
#20 display-translation-status-1804702-20.patch6.83 KBbalintk
#3 locale-download-ui-1804702-3.patch12.79 KBSutharsan
#3 locale-download-ui-3.png80.96 KBSutharsan
#2 locale_download_ui.patch70.56 KBJose Reyero
#2 locale_download_ui-diff.txt16.07 KBJose Reyero
#2 locale_download_ui_01.png52.49 KBJose Reyero
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +sprint
Jose Reyero’s picture

Status: Active » Needs work
FileSize
52.49 KB
16.07 KB
70.56 KB

This is a patch, based on #1804688: Download and import interface translations
that adds a minimal UI (language list, update button, settings form)
The second file is the diff to the other patch.
Attached screenshot.

locale_download_ui_01.png

Sutharsan’s picture

This is an separate patch to share my code for a translation status ui. It also includes a new form for the translation update settings.

locale-download-ui-3.png

Gábor Hojtsy’s picture

Assigned: Unassigned » Bojhan
Status: Needs work » Needs review

So these are two separate patches then? :) Do we see any way to merge the efforts with the best approaches? :) @Bojhan any feedback on the screenshots? :)

Gábor Hojtsy’s picture

Issue tags: +feature freeze

Add feature freeze tag.

Gábor Hojtsy’s picture

Also, Bojhan rightfully points out there was existing discussion at http://drupal.org/node/1029554#comment-4866138 onwards about this UI, so we know have 3 UIs competing then, two of them with implementation and one more that Bojhan actually agreed to earlier. Let's untangle this :D

Sutharsan’s picture

It was not my intention to make an new UI, I just made a quick and dirty one that helped me as developer. I gave Bojhan's latest design a try (http://drupal.org/node/1029554#comment-6121064) but it took me too much time and I went back to the 'download and import' code.

Status: Needs review » Needs work

The last submitted patch, locale-download-ui-1804702-3.patch, failed testing.

balintk’s picture

Assigned: Bojhan » balintk

I will go ahead, and try to come up with an implementation for the UI using Bojhan's design and the code that has been posted here.

Sutharsan’s picture

Assigned: balintk » Bojhan

I will include the code of the update settings form in #1804688: Download and import interface translations because it is needed there in the test scripts.

balintk’s picture

Assigned: Bojhan » balintk

Assigning the issue to myself again.

Gábor Hojtsy’s picture

Copy-paste of @Bojhan's latest design thinking from #1029554: Translations update feature user experience where all the UI discussions happened. Marked that as duplicate of this one.

I have updated the screens for the new simplified scenario, I still want to do a unified updates page - but in case thats out of scope a separate page to do this is fine.

Its basically a table, that only shows the language, clicking/entering upon Details you get more information specifically which module is getting updated.

A message on the status report page.

balintk’s picture

How should the "This applies language updates to" section work?

I suppose it should be updated dynamically whenever we select a new language from the list. Clicking on Details next to a language shows which modules have updates in that certain language, and the "This applies language updates to" section should display the list of modules which have updates in any of the selected languages.

Can someone please clarify this?

Gábor Hojtsy’s picture

@balintk: either that or it would be a summary of all the modules for all the languages that might be updated (which is what the design explicitly shows). It might be a not well explored area and mispresented since no language is selected.

Schnitzel’s picture

just wanted to mention, that the code posted in this patch is also contained in the latest patch of: #1804688: Download and import interface translations
not sure if this is how it should be.

Jose Reyero’s picture

Well, my patch in #2 (really simplified UI) was my understanding of what we were trying to get, as it is -kind of- more consistent with the module updates page and the screenshot in #12 in case we need to merge it later.

Anyway, merging with module updates page I think should be postponed for later. The basic question though is whether we want to see/select updates by language or by module. Improvements to the UI can very well be a follow up patch.

Whatever, could we settle on any (please as simple as possible one) to be able to just test the feature and make progress with #1804688: Download and import interface translations

One question, if we are still trying to merge both features (module update, translation update). Are we considering proposed translation downloads get automatically out of sync (obsoleted) as soon as we update any of the modules?

One more note, just for the record: I still see some screenshots inspired on what I did for l10n_update module which, to say it clear, was a TERRIBLE AWFUL UNUSABLE MISTAKE.

Bojhan’s picture

I dont think we need to merge it with modules right now, I also do think we need to determine a MVP option.

Sutharsan’s picture

Agree that we can postpone merging this UI with the module status till later (if we merge them at all).

@Schnitzel, the code in this patch should not include code from #1804688: Download and import interface translations. Except the hook_menu implementation of course. It would be good idea to include the status message on the status report page in this issue too.

My understanding of the UI in #12 is:

  • It includes a list of translatable languages with checkboxes per language.
  • No list of modules with checkboxes is included in the UI. Translation updates on a per module basis was considered to be a risk for inconsistent translations in the discussion in #1029554: Translations update feature user experience.
  • Clicking on 'Details' toggles an area with details of available updates for this language ('This applies language updates to: ...'). The details area is located directly below the appropriate language row.
  • Only the languages which are not 100% up to date are displayed. Up to date languages are hidden.

My suggestions extending the design:

  • A text which displays how long ago the status was updated, including a link to refresh the status. Similar to the Module Update interface.
  • Add to the details area the modules for which no translations were found (e.g. custom modules, features, new module releases). Alternatively show this information separately below the table.
  • Enable all language checkboxes by default. Or at least enable the checkbox when there is only one language in the list.
Sutharsan’s picture

For info: In #1804688: Download and import interface translations I posted an new patch in which the UI is now stripped to a minimum. The status locale.page.inc code in that patch will be a good starting point for the code of issue. The patch in this issue should be stand-alone and the two will therefore obviously overlap. But may the first RTBC patch win ;)

balintk’s picture

I have been working on this interface and would like to submit this interim patch containing the work that has been done so far. Sutharsan's code was the starting point for this.

It would be really nice if you could give me some feedback about the code and solutions. I introduced a new theme method for rendering the list of modules with available updates (and modules which updates were not found for). It seemed reasonable to me, but it would be great if someone could confirm that it was a good idea.

Current status of the UI:

 Available translation updates

Outstanding tasks:

I'll try to summarize the todos, please let me know if I missed something.

  • As you can see, the project list is shown in the table without the Details link that toggles that information. To be honest, I'm not sure anymore if the toggling is actually needed, moreover, I'm not sure if the list containing all projects is needed below the table. I think the user can get a nice overview per language, and I don't see the additional value in showing the summarized list too. It's just my feeling, and I might be completely wrong. What do you think?
  • List of projects which updates were not found for is missing. At the moment I'm not sure how to gather this information, but I will look into this.
  • Message for the status report page.
balintk’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, display-translation-status-1804702-20.patch, failed testing.

Bojhan’s picture

The image is too small to review.

balintk’s picture

Sorry, that image was meant to be just a thumbnail and a link to a bigger one that I uploaded with Skitch. Attaching the large image here.

Gábor Hojtsy’s picture

Fixed link to bigger image in #20.

Gábor Hojtsy’s picture

Title: Display translation status » Display interface translation status
Sutharsan’s picture

@balintk, Each project-language object can have a 'type' property with these values:

  • 'remote' or 'local': A translation is available at the translation server (remote) or in the local file system (local)
  • 'current': The translation is up to date.
  • 'empty' or missing: No translation was found. A translation may not be available (e.g. current 8.x core) or the remote server can not be reached. In the latter an error message (currently "One translation files could not be checked. See the log for details") will be displayed to the user.
Sutharsan’s picture

You could differentiate between 'updates' and 'not_found' like this:

  foreach ($status as $project_id => $project) {
    foreach ($project as $langcode => $project_info) {
      if (!isset($project_info->type)) {
        $projects_to_update['all'][$project_info->project] = $projects_to_update[$langcode]['not_found'][] = array(
          'name' => $project_info->name,
          'version' => $project_info->version,
        );
      }
      elseif ($project_info->type != 'current') {
        $projects_to_update['all'][$project_info->project] = $projects_to_update[$langcode]['updates'][] = array(
          'name' => $project_info->name,
          'version' => $project_info->version,
        );
      }
    }
  }

I doubt if we need an 'all projects' list on the bottom. Project names are already listed at the respective languages. The version numbers add a lot of noise to the screen, I don't think that they are needed all the time. We could hide them under a link 'details' which toggles a bullet list:

  • drupal (8.2) update: 8.2 (April 1, 2013)
  • echo (8.x-1.4) update: 8.x-1.9
  • xhtml (8.x-1.2) update: 8.x-1.3

The part "Updates were not found for: ..." could be reduced to one line: "No updates were not found for 5 projects." Under the 'details' link we can again show the details:

Listing a file location is very helpful for debugging. The text about dev releases is fixed and applies to all dev's. Note that version numbers are optional.

Sutharsan’s picture

Screenshot to illustrate available and missing updates:
Only local images are allowed.

balintk’s picture

Some progress:

  • List containing project updates in all languages has been removed from the bottom.
  • Summary of available updates contains module names only in an inline list.
  • Summary of updates were not found contains only the number of projects.
  • Details links are available after summaries for accessing to more detailed lists.
  • Theme function that renders the output for each language row has been rewritten.

Screenshot thumbnail

The error messages are still missing from the detailed list as well as the dates with the new recommended versions.

I think it's an interesting question what should we do with those language rows where there is no update available and the summary only shows projects which updates were not found for. Disable the checkbox? Maybe not a good idea if we consider UX-issues.

This patch unfortunately can't be applied with the latest patch from #1804688: Download and import interface translations, it needs the older patch, but I will reroll it soon.

Sutharsan’s picture

Sutharsan’s picture

I have the following suggestions, which I also mocked in the screenshot below.

  • No Languages label
  • Different table heading texts
  • Move the 'details' links to an operations column. Strictly this may not be an operation, but links on the right it is a known pattern in the Drupal interface.
  • Different the text of the details links
  • Combine the two details links per language into one.
  • Different text in the 'description column' I found "This applies language updates to ..." confusing. But have not done any user tests with it.
  • Different text on the submit button

Sutharsan’s picture

Re-rolled against the #60 patch of #1804688: Download and import interface translations. In #31 I accidentally included some code that doesn't belong in this issue.

Sutharsan’s picture

Oops, forgot to include the js-file.

YesCT’s picture

I'm testing this and will update with results soon.

YesCT’s picture

I didn't get done with this testing. Someone else can jump in to do it, or I'll come back to it later today.

balintk’s picture

I went through Sutharsan's checklist in #32. Also made some improvements on the way. Language rows now don't have a checkbox when no updates are available but there are modules which updates were not found for (I discussed this with Sutharsan on IRC).

display-interface-translation-status-1804702-37-1-small.png display-interface-translation-status-1804702-37-2-small.png

Unfortunately, it doesn't look really good when we toogle the details. The table column jumps to the left because the content needs wider space. In my opinion we should use the same interface as the new module page uses. I discussed this with Gábor, he also thinks we could give it a try. What do you think?

Gábor Hojtsy’s picture

I think we'll be able to tweak the UI of this page for quite a while, however we should have a usable UI in core sooner than later. We can do user testing of the user interface and figure out how it works or not.

As for this specific UI in the latest patch, I think @Bojhan's intention was to leave off the languages which don't have updates, however, I see the reason to include "debugging" information for site builders/users as to what kind of issues there are with those IF things are missing updated vs. merely just not having anything new to download/import. So removing the checkbox is still better than making it disabled I think. Including this information in a separate table and/or under the table could become confusing I think. So not entirely sure what would be the best, but these are the kind of things we can tweak IF we believe we have a base implementation that is not way off the mark. :)

@Bojhan what do you think of the latest patch/UI? It looks similar to your design with some tweaks :)

Bojhan’s picture

I spoke with Sutharsan, I am not really a fan of showing "There is nothing, wohoo!" kind of debugging information. But it seems really important for many current users of i18n_update so I am going to defer to that for now.

A couple suggestions:

  1. Is it possible for this to use the module page pattern? At least in terms of collapsing, having a > next to description - rather than "Show more details".
  2. Lets try to shorten the text, its a bit much to scan for status. E.g. "No translations found for 3 projects" -> "No updates" - the whole, 3 projects, is pretty useless information anyway when you have a normal website you have 20/30 modules and no one remembers their module count. If they want to know more they will collapse this information
  3. Same for "Translation updates available for:" can be removed. If there are modules listed, it has updates, that should be relatively obvious since you are on the updates page, and the others list "none found".
  4. Can the check manually message, have some whitespace below it? It is now attached to the table making it hard to get noticed
Gábor Hojtsy’s picture

@Bojhan: looks like you independently got to the same idea that @balintk suggested this weekend and we discussed (see posted in #37). I told him not to get into a process where new things are worked on without buy-in, but looks like there is buy-in, so do you agree we should continue with making the page look like the new modules list then? :)

Bojhan’s picture

@Gabor Just the toggle > should be the same, all the stylistic parts of the module page you can leave out.

Gábor Hojtsy’s picture

FileSize
33.41 KB

@Bojhan: well, the reason the "module page pattern" came to discussion earlier was that as can be seen from the first and second screenshot in #37, the width of the table cells change dramatically when you expand/collapse the details. So the thinking was it would make more sense to include the summary text and make it the expand/collapse initiator (like on the modules page), and then it almost exactly looks like the modules page, you have a checkbox, a short name and a short summary which serves as a collapse. The only difference would be the tableselect (select box for the whole table in the table header) (which maybe makes it non-standard to apply the module page pattern altogether).

ModulesPage.jpg

Jose Reyero’s picture

I think the modules update page is really a bad inspiration for this one, a few reasons:
- It splits core/modules/themes, we don't mind that for translations.
- it provides extensive information about current/available versions, we don't mind that for translations.
- It focuses on module updates one at a time, just because updating modules is a lot more complicated / dangerous process.

Really, this 'module updates' page is where all the problems come from. We don't need that much information for translation updates.

Translation updates are 99% of the time: "Yes, get me all updates.", the other 0.9% is "I want to update this language but not this other" and the remaining 0.1% maybe "Oh, I have nothing to do today, I want extensive information on minor versions of modules that have available translation updates (And still I would be pissed off because I cannot see what I really would like to see, which is the exact strings that are going to be updated)"

So the point: all this information is useless for translation updates. If there's anything in the patch still not working, just cut it off (that may actually improve usability).

Gábor Hojtsy’s picture

@Jose: yes, the proposal was to cut the UI off even more and only include a very short summary for each language with more information as dropdowns, taking cue from the new modules page for visual presentation (the short summary being the expand/collapse trigger instead of a details link, showing the details under the short summary).

balintk’s picture

Issue summary: View changes

Updated issue summary.

balintk’s picture

Issue summary: View changes

Added screenshot to issue summary about user interface changes.

Sutharsan’s picture

FileSize
12.87 KB

Re-rolled against latest patch of #1804688: Download and import interface translations.

Sutharsan’s picture

A new proposal for the translation status on the status page, based on the new module page layout. I've tried to reduce the text and the detailed information to a minimum.
locale-translation-status-page.png

The attached patch includes status messages on the Status report page. Screenshots of the various states are attached for evaluation.

locale-translation-status-available.png

locale-translation-status-not-found.png

locale-translation-status-ok.png

locale-translation-status-unknown.png

Jose Reyero’s picture

Well, this one is looking much nicer :-)

Though I agree with @Bojhan (#39) about all these 'No updates found for' being pointless, I have nothing else really agains them if they're already in the patch and working.

Just one question: In the case there are no updates for a language, how do we display it?, is the language still in the list? (with checkbox?)

Sutharsan’s picture

A new version with Bojhan's comments in #39 and #41 applied.

locale-translation-status-page-47.png

Sutharsan’s picture

The 'no updates' is for all those people asking (or banging themselves on the head) why is my module not translated. In other words, for debugging.

I propose that languages which don't have updates and all updates are found, will not be listed. Languages which don't have updates but have 'no updates' are listed, but don't have a checkbox.

Bojhan’s picture

We probably should retain the summary, of what modules do have available updates. What about like this?

updates-i18n-shortdescription.png

Ignore the "dutch" styling, mistake.

YesCT’s picture

I prefer the one from #46 But take off the "No updates found for 1 projects" *when* there are updates found for some. When there are no updates for any, just say "No updates found". That shortens the text both in the case of updates found and no updates found.

I think the one in #48 would lead to having to click on all the language names to expand them to see whether there is information there I would want to see. And it looks unbalanced...

Are the warning messages in #46 shown on the same page that the screen shot is for?
It says "See the -link-Available translations updates-link- page for more information."
If the message is shown on that same page, I think that is awkward.

If the warnings are showing on other pages, like the language config page where langauges can be added, or the translation ui customize page, then it makes sense to link to the report.

I think since there is this little confusion about that info being under report, it might help to have the warning be:
"See the -link-Available translations updates-link- report for more information."

YesCT’s picture

Listing the actual module names (or core?) in the collapsed summary is even better.

Sutharsan’s picture

@YesCT, the screen shot is admin/reports/translations (Available translation updates). The messages are on admin/reports/status (Status report).

Jose Reyero’s picture

@YesCT

I prefer the one from #46 But take off the "No updates found for 1 projects" *when* there are updates found for some. When there are no updates for any, just say "No updates found". That shortens the text both in the case of updates found and no updates found.

+1

@Bojhan #50
At the very least there should be a message about why there's no checkbox for the language, I guess.
Also, grayed out (disabled) checkbox would be better than missing one IMO.

I think I would prefer to see "how many projects have available updates" (which at least lets me figure out whether this is going to be a long/short update or how many downloads?) instead of a trimmed module list which tells me nothing actually (and looks ugly when expanded).

But again, I'd be fine with whatever is currently in the patch if working and similar to what we are discussing. We are doing unneeded polishing on a page that may still go through some changes if I've understood right, like future merging with module updates, etc...
(Not to mention I believe this doesn't really belong here, but to the translation status page).

Bojhan’s picture

@Jose Stop looking only at the images :P I noticed below you should ignore that no checkbox styling it was a mistake.

Sutharsan’s picture

I talked with Bojhan in IRC. He noted that the fist line 'Dutch' was mistake. Disregard that line. Up-to-date languages are not listed.

Gábor Hojtsy’s picture

+1 to not list up to date languages :)

Sutharsan’s picture

Assigned: balintk » Sutharsan

Working on the patch. I expect to deliver tomorrow.

Sutharsan’s picture

Issue summary: View changes

Updated summary

Sutharsan’s picture

An intermediate patch with status display. Checkboxes not working. Next is: working checkboxes, styling, completion of todo's an tests. The screenshot shows the current state of the status page.

locale-translation-status-page-59.png

Sutharsan’s picture

Issue summary: View changes

Updated summary

Sutharsan’s picture

Issue summary: View changes

Updated todo list

Sutharsan’s picture

This is a working interface.
Still needs to be integrated in the existing tests. It can use some css and js tuning. I have used the Module page code as an example, but I think it was not the best example I could have used.

locale-translation-status-page-60.png

Sutharsan’s picture

Status: Needs work » Needs review

Oh, yes. It is ready for review.

Status: Needs review » Needs work

The last submitted patch, display-interface-translation-status-1804702-34.patch, failed testing.

Gábor Hojtsy’s picture

@Sutharsan: if you reused the module page CSS/JS that is good in terms of it already getting accessibility review, so not needing it again here :) The UI looks good to me.

Bojhan’s picture

Status: Needs work » Needs review

Passes UX Review :) I'm happy with it.

Sutharsan’s picture

@Gábor Hojtsy, I which I could have re-used the module page code. Module pages uses its own html to build the form using theme_table and its JS is complex and over the top for the use on this page. Using the tableselect element as I did gave me the check-all function for free, resulted in much simpler code. But tableselect has its flaws, and looking at the issue queue tableselect is still work in progress. It has lost the mobile features to show/hide low priority colums, which I will add to the todo list. So, it may need an accessibility review.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Ok, adding tag then.

Sutharsan’s picture

Including tests, including show/hide low priority columns, improved css.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

Sutharsan’s picture

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Should be a testable patch now that #1804688: Download and import interface translations is committed :) Uploaded without the do-not-test suffix. No change.

Status: Needs review » Needs work
Issue tags: -Needs accessibility review, -D8MI, -sprint, -language-ui, -feature freeze

The last submitted patch, display-interface-translation-status-1804702-67.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
Issue tags: +Needs accessibility review, +D8MI, +sprint, +language-ui, +feature freeze
Sutharsan’s picture

The patch applies fine locally and passes tests too. Trying another test run.

Lars Toomre’s picture

A few observations from reading through the patch in #69:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateInterfaceTest.phpundefined
@@ -0,0 +1,107 @@
+ * @file
+ * Definition of Drupal\locale\Tests\LocaleUpdateInterfaceTest.

Should be 'Contains' instead of 'Definition of'. Repeated below in patch.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateInterfaceTest.phpundefined
@@ -0,0 +1,107 @@
+  }
+}
\ No newline at end of file
diff --git a/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php

Missing newline at end of file.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -656,7 +740,70 @@ function locale_translation_status_form_submit($form, &$form_state) {
+ * @param string $version
+ *   Version of the project.
+ * @param string $remote_path
+ *   Remote path where the translation file was tried to load from.
+ * @param string $local_path
+ *   Local path where the translation file was tried to load from.
+ */
+function _locale_translation_status_debug_info($source) {
+  $remote_path = isset($source->files['remote']->uri) ? $source->files['remote']->uri : '';

@param directives do not match function variable. Also missing a @return directive.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -656,7 +740,70 @@ function locale_translation_status_form_submit($form, &$form_state) {
  * Default theme function for translation edit form.
+ *
+ * @see locale_translate_edit_form()
+ * @ingroup themeable
  */
 function theme_locale_translate_edit_form_strings($variables) {
   $output = '';

Missing @param directive.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -689,3 +836,73 @@ function theme_locale_translate_edit_form_strings($variables) {
+ * Default theme function for translation status information per language.
+ *
+ * @see locale_translation_status()
+ * @ingroup themeable
+ */
+function theme_locale_translation_update_info($variables) {
+  $description = $details = '';

Missing @param directive.

Gábor Hojtsy’s picture

@Lars: "Should be 'Contains' instead of 'Definition of'. Repeated below in patch." Erm, when did the coding standards for file level docs changes for tests? Tests I randomly opened from my Drupal 8 checkout all used "Definition of". Any docs of this change?

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summray

YesCT’s picture

Updated the steps to test in the issue summary.

Here are some updated screen shots. Including new before shots since #1804688: Download and import interface translations was committed.

1. (no patch) Link to the report page this patch provides

(Enable locale, add a language, see the link to the translation status report).
di-s01-8-2012-12-08_0036.png

2. (no patch) admin/reports/translations

di-s02-8-report-2012-12-08_0037.png

3. (no patch) after manual check

di-s03-8-manual-2012-12-08_0038.png

4. (patch) translation status report page with no updates

singular project, no checkbox, no button
Q1: why is the block project listed? there are no .po files. I grepped for them.
di-s04-report-2012-12-08_0054.png

5. (patch) expanded

di-s05-expanded-2012-12-08_0056.png

6. (patch) check manually

clicking the check manual, is the same as without the patch (see 3.)
di-s06-manual-2012-12-08_0057.png

7. (patch) admin status report page

Q2: why is it a warning that no updates are available? That seems like it would be "up to date" and all is well.
di-s07-admin_status_report-2012-12-08_0308.png

8. (patch with tamper) shows updates available and lists module names

checkbox to update is checked by default. good.
di-s08-tamper-check_manually-2012-12-08_0320.png

9. (patch with tamper) expanded shows each module with the date of the update, and the projects that do not have updates

di-s09-tamper-expanded-2012-12-08_0320.png

10. (patch with tamper) general admin status report page

shows updates available, with link to translations updates report page. warning makes sense to signal updates are available.
di-s10-tamper-admin_status_report-2012-12-08_0322.png

11. (patch with tamper) after update translations, shows plural in no updates phrase

Q3: why does it say no updates found for 3 projects? I expected it to say no updates found for 10 projects (the 7 it just updated and the 3 that do not have updates yet)
di-s11-after_updates-plural-2012-12-08_0325.png

12. (patch with tamper) expanded

Q4: regarding Q3, I'm thinking these are the projects cannot check if they have translations (or updates) because they are dev or missing release info.
di-s12-expanded-2012-12-08_0326.png

Suggestion

No updates found for 3 projects

    Block (8.x-dev). No translation files are provided for development releases.
    locale_tamper (no version). File not found at http://ftp.drupal.org/files/translations/8.x/locale_tamper/locale_tamper-.nl.po nor at translations://locale_tamper-.nl.po
    Wysiwyg (7.x-2.1+28-dev). No translation files are provided for development releases.

Suggestion 1: Reword phrases like
"No updates found for 3 projects"
to
"Cannot check for translation updates for 3 projects"

Q5: what does file not found mean? that there is no translation for that project? Is having no translation a warning? Maybe a project is such that a translation does not make sense? Or do we expect every project to have translations and so we want to alert that there is no po file anywhere (local, or served)?

Suggestion 2a (Work/Wording in progress): Reword:
locale_tamper (no version). File not found at http://ftp.drupal.org/files/translations/8.x/locale_tamper/locale_tamper... nor at translations://locale_tamper-.nl.po
to
locale_tamper (no version). File not found on the community translation server at http://ftp.drupal.org/files/translations/8.x/locale_tamper/locale_tamper... nor on a custom server at translations://locale_tamper-.nl.po

Suggestion 2b (Work/Wording in progress):
or to
locale_tamper (no version). Translation file not found.

I'm not sure about either 2a or 2b. I think this needs input from people more familiar with the inner workings of serving translations and ways sites can be configured to get them. For example, perhaps a site can be set to point to a server other than localize.drupal.org

YesCT’s picture

FileSize
131.3 KB

Trying to get "all translations up to date"

Looks like there is test coverage for this case, but I was trying to test it manually...

+++ b/core/modules/locale/locale.pages.incundefined
@@ -595,46 +595,130 @@ function locale_translate_settings_submit($form, &$form_state) {
+  if (!$languages) {
+    $empty = t('No translatable languages available. <a href="@add_language">Add a language</a> first.', array('@add_language' => url('admin/config/regional/language')));
+  }
+  elseif ($status) {
+    $empty = t('All translations up to date.');
+  }
+  else  {
+    $empty = t('No translation status available. <a href="@check">Check manually</a>.', array('@check' => url('admin/reports/translations/check')));
+  }

Here I see something for All translations up to date.

But I dont have steps to test that can show that.

Disabling tamper

Disabling tamper lead to errors like

Notice: Undefined index: wysiwyg in locale_translation_status() (line 620 of core/modules/locale/locale.pages.inc).
Notice: Trying to get property of non-object in locale_translation_status() (line 620 of core/modules/locale/locale.pages.inc).

I tried disabling block module and tamper to see if I could get a "All translations up to date".

gives in the expanded details:

    Block (8.x-dev). No translation files are provided for development releases.
    (no version). File not found at http://ftp.drupal.org/files/translations/8.x/locale_tamper/locale_tamper-.af.po nor at translations://locale_tamper-.af.po
    (7.x-2.1+28-dev). No translation files are provided for development releases.

The name for tamper project (and wysiwyg project which tamper provided?) are missing, but I think the whole lines should be gone.
Disabling the block core module, did not make the block translation line go away.

uninstall projects

after uninstalling block and tamper,
in the expanded details it says:

No updates found for 2 projects

    Breakpoint (8.x-dev). No translation files are provided for development releases.
    (7.x-2.1+28-dev). No translation files are provided for development releases.

di-u01-2012-12-08_0439.png

YesCT’s picture

I tried an install from scratch (sudo rm -r sites; git checkout sites;)
To try and get to a state where all the translations were up to date.

1. fresh site reports page

di-t01-status-no-2012-12-08_0447.png

2. I disabled block module, and then uninstalled it, and then enabled locale.

I tried looking at the general admin status report to see if it would say all translations up to date. It did not have translations there one way or the other. which I guess is ok.
di-t02-2012-12-08_0451.png

3. I added a language.

(8 of 8 strings imported flashed, which it did previously I just didn't mention it...)
and now the status page says no update found and links to the translation status report page... which shows no updates found for breakpoint!
di-t03-added_spanish-2012-12-08_0453.png

4. it's showing the first enabled core module, but not all of the enabled core modules.

di-t04-first_core-2012-12-08_0456.png

YesCT’s picture

I did a video and captured the screens off that to see what they were saying.

This should probably be a follow-up of another issue, not this one. I'll be happy to open the issue.

Why does it say downloading and importing translation files? ... there are none to download.
Perhaps it is more accurate to say "checking for translations to download" until it finds some to download or import?

di-why-2012-12-08_0512.png

Then why does it say "Completed 8 of 8. 100%"?
completed what? checking for translations? downloading? importing?
8 of 8 what? not projects; I have more than 8 core projects enabled.

di-8of8-2012-12-08_0512.png

I search in the UI translations screen and limit the search to translated strings and it says zero.

Lars Toomre’s picture

Re #74: The comment about 'Contains' come from [#1354#classes]. I think the change from 'Definition of' happened back in July, but apparently Test classes are to be treated differently since they have their own docs page. Hence, I am now am unsure of what the description should be.

Sutharsan’s picture

@YesCT, Answers to your questions in #67:
Q1 "why is the block project listed?" This is raw data from update_process_info_list(). It should read: 'Drupal core'.
Q2 "why is it a warning that no updates are available?" This is a gray area. In case of development releases it is a for information, the site owner can't do anything about it. In case of custom modules it can help the site owner to place the po file in the right location or it can be ignored if the module does not introduce translations or has 'manual' translations and therefore does not bring a po file. In case there is no po file generated by l.d.o it should give the site admin an indication what is wrong, reset the l.d.o translation, try later or can report it in the infrastructure issue queue. The latter is the most reported problem with missing translations in the l10n_update issue queue. And with that my biggest motivation to add this debug data. I have no objections to change this to an 'Ok' state on the Status report page instead of a warning. It will, and can be, ignored most of the time.
Q3 "why does it say no updates found for 3 projects?" It checks 10 projects, finds and imports 7 translation files and fails to find translations for 3 projects.
Q4 "regarding Q3, I'm thinking these are the projects cannot check..." Correct.
Q5 "what does file not found mean? " See my answer on Q2. We don't expect every project to have a translation. We could improve the fallback strategy of finding translations for dev releases and a contrib module could offer a series of checkboxes to ignore missing translation of (custom) module. But as far as the scope of this issue is concerned, we should find for a 'keep it simple' description which covers all cases.
Re. rewording: I have avoided using "the community translation server". l.d.o is the default translation server but can be replaced for example in case of distributions which have their own server. And l.d.o calls itself "Drupal Translations" Perhaps better to call it "Drupal translations website" instead of "... server".

More comments and new code will follow.

YesCT’s picture

That does help clear up some things.
I agree for now, keeping simple and general is a good idea.
We determine what exactly to file follow-up issues for considering future fancy things. :)

To me, an "update" means that the system already had imported a translation and it is looking to see if an updated, translation with a more recent timestamp, is available. But currently, the word updates is used also to mean a bit more than that. I'm not sure if that is a todo yet, a follow-up, or a wont fix. Just clarifying and noting it.

I'm going to open issues for the unrelated error in #76 screenshot 11 about destination directory.
And the question in #79 about the 8 of 8

For core, is it one .po file for all the core modules? Or is it a tar file with a few files? I can check...
I looked at http://ftp.drupal.org/files/translations/7.x/drupal/drupal-7.17.de.po
it's one file and not specific for core modules. So this makes even more sense that it will just say "Drupal core"

YesCT’s picture

YesCT’s picture

YesCT’s picture

Status: Needs review » Needs work

I think once we get the new code @Sutharsan mentioned in #81 I'll put up a live site demo to make accessibility reviews easier.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
31.27 KB

Ok, new code it is ;) In #67 I already included all of the accessibility specials that were added to the Modules table, but I may have overlooked something. So, I don't expect the accessibility review to be a problem.

Status: Needs review » Needs work

The last submitted patch, interdiff-1804702-75-86.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

(oops the interdiff was named .patch instead of .txt, so marking needs review since the patch passed the bot)

YesCT’s picture

Updated screenshots (similar steps as in #76, reusing those step numbers for before and after coordination)

1.

(Enable locale, add a language, see the link to the translation status report).
[...]

4. (patch) translation status report page with no updates

singular project, no checkbox, no button
Q1: Didn't we decide earlier to not list the number of projects if there were no updates? (Decide is too strong of word. Suggested in: #54) I consider this a nit, and ok with won't fix, more discussion, or a follow-up.
di-86-s04a-2012-12-12_0232.png

5. (patch) expanded

Nice. Latest patch fixes this to just say "core"
di-86-s04-2012-12-12_0224.png

6. (patch) check manually

Q2: separate issue? Says 3 of 3 completed. Then says updates checked for "one project".

7. (patch) admin status report page

fixed in the most recent patch: no updates found now has no warning (which makes sense because it's not really something the site builder can fix.)
Q3: suggest no green color. have it be like the others around it, like: Upload progress: not enabled.
suggest reserving the green for: "all up to date" when every project has a .po file and it is up to date.
I consider this a nit and could be done in a follow-up. might make a nice novice one.
di-86-s07-status_report-2012-12-12_0243.png

7.5 get tamper

[download tamper, enable module, add another language: German]
Strangely some of status below the progress bar show the html em tag around the project (placeholder ctools), some have it italicized.
Q4: needs unrelated issue opened, or is it tamper specific?

8. (patch with tamper) shows updates available and lists module names

admin/reports/translations/check (check manually)
admin/reports/translations shows
checkbox to update is checked by default. good.
di-86-s08-updates_collapsed-2012-12-12_0309.png

9. (patch with tamper) expanded shows each module with the date of the update, and the projects that do not have updates

di-86-s09-2012-12-12_0319.png
Q5: why are there updates shown for spanish? I thought tamper only included Dutch and German.
C1: separate compulsive thought: would be nice to list the project po files that were checked and up to date with their dates. But this clutters and is not needed. Maybe as a collapsed inside the collapsed? Ignoring this thought is ok. It was in previous versions and taken out to improve the ui.
Q6: are those all the modules downloaded (listed on the Extend page), or just the ones enabled?
Investigating, back to install again, but no enable tamper (tamper is downloaded to /modules):
Answering my own question: it's just the enabled ones... wait
Q7a (with no tamper): Well, I downloaded and enabled devel, and it's not showing up there, why Not even warning that the translation files are not provided for dev releases. (with no tamper)
With tamper, tamper shows up as no updates (so does devel, but tamper comes with devel)
Trying a module not in tamper, but with tamper enabled. Date (downloaded to /modules but not enabled). Does not show: ok.
Q7b (with tamper): Trying a module not in tamper, but with tamper enabled. Date and admin menu (enabled). Does not show, why? Should be listed with "files are not provided for dev releases"

10. (patch with tamper) general admin status report page

shows updates available, with link to translations updates report page. warning makes sense to signal updates are available.
di-86-s10-status_updates available_warning-2012-12-12_0405.png

11. (patch with tamper) after update translations, shows plural in no updates phrase

During update:
Q8: progress bar during update translations says X of 23. why 23? there are only 10 projects... is that counting the core project individually?
After update:
di-86-s11-2012-12-12_0420.png

Suggestion 1: Reword phrases like
"No updates found for 3 projects"
to
"Cannot check for translation updates for 3 projects"

Q9: bringing this up again. should we make this change in wording? would be nice to have another opinion here.
why does this bother me.. because after updating those other 7, there are also "no updates found" for those 7! It's just that we dont care/need to list them or mention that.

See Q1. Maybe removing the count solves this?

12. (patch with tamper) expanded

di-86-s12-expanded-2012-12-12_0423.png

Summary

Only really needing addressed: the Questions 5 through 7 under step 9 to see if there is a potential bug.
Depending on the answer to those, this looks ready to me.

The other questions I have are non-blocking for sure, ranging from me just documenting thoughts, to things that can be follow-ups.
We have UI sign off in #64 and it's pretty much the same now.
We are not expecting accessibility issues (See #86)
Maybe could use a semi-final code review of the patch in #86.
Is this reviewable by @webchick or is this kind of blocked on #1861360: Refactor localization update test so people can just enable the test module to test ?

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary with steps to test (previous dependency was committted)

Sutharsan’s picture

Q1 about "No updates found for 1 project"

I'm fine with changing this to "No updates found" but we could make it a bit stronger: "Untranslated projects" or "Missing translations"

Q2: separate issue? Says 3 of 3 completed. Then says updates checked for "one project".

Separate issue, please. "3 of 3 completed" is because the batch is executed with 3 steps. The number is only partly related to the number of projects. To avoid confusion, we can remove the number. This can be applied to all check/download/import batch texts of locale module.

Q3: suggest no green color

Agree, change to 'for info'/gray.

Q4: needs unrelated issue opened, or is it tamper specific?

It's a minor bug. This happens during translation import. It can be combined with the Q2 follow-up issue. For the record, the displayed text is: "Importing translation file: ctools-7.x-1.2.de.po (88%)"

Q5: why are there updates shown for spanish?

Apparently you have added Spanish. Locale Tamper only mocks modules, no languages.

would be nice to list the project po files that were checked and up to date with their dates.

Have you used Locale Update module with 40 projects and 5 languages? The screen is flooded with messages. Trust me, when you use Locale Update module for a bit longer, you only go to the update page to look for the amount of yellow rows (updates available) and immediately hit the 'update' button. We agreed on an interface with only the must have information. Only slowly Drupal is moving away from the cluttered interfaces with many, many unused details. Lets keep moving into this direction, and not go backwards.

Q6: are those all the modules downloaded (listed on the Extend page), or just the ones enabled?

On the settings page (admin/config/regional/translate/settings) you can choose to include disabled modules too. By default only enabled modules are translated.

Q7a (with no tamper): Well, I downloaded and enabled devel, ...
Q7b (with tamper): Trying a module not in tamper, ...

Forget Locale Tamper module. It's already way past it's expected life time ;) It works badly with existing modules and fails miserably when you disable the module. Just install locale and devel, add one language and you will see both core and devel listed under 'No updates found ..."

Q8: progress bar during update translations says X of 23. why 23? there are only 10 projects... is that counting the core project individually?

See Q2. Import = check remote + check local + compare results + fetch status + download remote + import translation + compare results + update status = many steps. Sometimes one action for all project, sometimes one action per project-language combination. Conclusion: much too much for a simple message.

Q9: bringing this up again. should we make this change in wording? "Cannot check for translation updates for 3 projects"

It did check, but it was not able to find any translation file where it expected one. If we show numbers (or names) it should match the expectation. If you enable for example 3 projects, and it reports to have 2 files imported, that raises a question. We should add something like "Could not find the translation for 1 project. See ... for details.", or less techy: "Could not translate 1 project. ..." (Q2 follow-up). When you add a language these numbers have little meaning, but not being able to translate a number of projects is not reported here either, but equally important to notify. But these notifications are not in the scope of this issue as they are part of the check/download/import process.

YesCT’s picture

Thanks for all those answers! The answer to Q5 made me smile. :)

I'm going to test this again:
me:

Q7a (with no tamper): Well, I downloaded and enabled devel, and it's not showing up there, why Not even warning that the translation files are not provided for dev releases. (with no tamper)

@Sutharsan:

Just install locale and devel, add one language and you will see both core and devel listed under 'No updates found ..."

Because I think I did *not* see devel. But I was testing it all at once and could have just been confused at that point.

YesCT’s picture

Status: Needs review » Needs work

enabled contrib modules are not showing when checking translation status.

steps to reproduce:

clean d8
apply patch
install
enable locale
add a language
check report (see core no update available)
download devel
enable devel
check report (check manually)
see devel not listed

YesCT’s picture

changing the setting at admin/config/regional/translate/settings
to " Check for updates of disabled modules and themes "

still does not show devel on the report.

YesCT’s picture

Status: Needs work » Needs review

Located why it was not working for me. I had gotten devel with a git clone of devel. That will not show on the available updates report (or on the translation status report). getting the tar.gz from d.o has devel show fine in both places. (Also, when I tried another module instead of devel, I tried ctools, the tar.gz from d.o of ctools, and that just doesn't work.)

these are debugging stuff and can be safely skipped watching, but just here for the extra curious
me trying ctools http://www.youtube.com/watch?v=-jULAj74tLE
me figuring out the diff between clone and the tar.gz download from d.o http://www.youtube.com/watch?v=FTykYKm-9aE

Summary

Summary, the patch in #86 works.
There are a few small things that can be follow-ups.

Needs review for a double check of the code

YesCT’s picture

I did a code review, looks good but I did fix some coding standards
and some of the fixes from #91
grey/INFO for status
no updates found -> missing translations. This means the number of projects makes more sense, so left that.

Q2 needs a follow-up

Q8 the translation import status messages. In irc Sutharsan showed me the work on that and it's really nice, but I'm not sure if a follow-up issue number for that exists yet or not. (Q4 is part of that)

Everything else is addressed/answered/wont fix/already has follow-up.

Updating the issue summary with screenshots:
review-2012-12-13_1446.png
and
no updates found 2012-12-13_1436.png

Next

Next: final code review and hopefully rtbc.

YesCT’s picture

Issue summary: View changes

update screen shots, remaining task list, add follow-up/related issues.

YesCT’s picture

Issue summary: View changes

Updated issue summary with recent screenshots

YesCT’s picture

oops. forgot the patch!

Gábor Hojtsy’s picture

@YesCT: for git deployed projects, you need to use http://drupal.org/project/git_deploy or the workarounds mentioned there. It is a symptom of how git checkouts include no version information. Update status module would not recognise those module versions either.

Sutharsan’s picture

Created follow-up issue #1866544: Improve Locale batch progress and result messages and added the code and video link.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I just reviewed the code in this patch. It generally looks pretty good. Mostly minor remarks:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateInterfaceTest.phpundefined
@@ -0,0 +1,104 @@
+/**
+ * @file
+ * Tests for Drupal\locale\Tests\LocaleUpdateInterfaceTest.
+ */

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -2,7 +2,7 @@
 /**
  * @file
- * Definition of Drupal\locale\Tests\LocaleCompareTest.
+ * Tests for Drupal\locale\Tests\LocaleUpdateTest.
  */

Is this right? I mean we are not testing the test :) Is this the current code style for test headers?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateInterfaceTest.phpundefined
@@ -0,0 +1,104 @@
+  function setUp() {
+    parent::setUp();
+    // We use German as default test language. Due to hardcoded configurations
+    // in the locale_test module, the language can not be chosen randomly.
+    $admin_user = $this->drupalCreateUser(array('administer modules', 'administer site configuration', 'administer languages', 'access administration pages', 'translate interface'));
+    $this->drupalLogin($admin_user);
+  }

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -51,20 +51,16 @@ class LocaleUpdateTest extends WebTestBase {
   function setUp() {
     parent::setUp();
+    // We use German as default test language. Due to hardcoded configurations
+    // in the locale_test module, the language can not be chosen randomly.
     module_load_include('compare.inc', 'locale');
     module_load_include('fetch.inc', 'locale');
     $admin_user = $this->drupalCreateUser(array('administer modules', 'administer site configuration', 'administer languages', 'access administration pages', 'translate interface'));

What is the significance of this comment? There is nothing happening around it with German?!

German is only added later. Maybe move this there?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateInterfaceTest.phpundefined
@@ -0,0 +1,104 @@
+    // Drupal core is probably in 8.x, but tests may also be executed with
+    // stable releases. Therefore we will just ignore it here to make sure we
+    // have a controlled environment.
+    $status = state()->get('locale.translation_status');
+    unset($status['drupal']);
+    state()->set('locale.translation_status', $status);

I'd add at the end "and tinker with prepared contributed modules only". Or something along those lines.

+++ b/core/modules/locale/locale.admin.jsundefined
@@ -40,6 +40,41 @@ Drupal.behaviors.localeTranslateDirty = {
+        $tr.find('.udpate-description-prefix').text(function () {
+          if ($tr.hasClass('expanded')) {
+            return Drupal.t('Hide description');

.udpate looks like a typo :)

+++ b/core/modules/locale/locale.pages.incundefined
@@ -595,46 +595,130 @@ function locale_translate_settings_submit($form, &$form_state) {
-function locale_translation_status_form($form, &$form_state) {
+function locale_translation_status($form, &$form_state) {

Given this keeps being a form callback, I'm puzzled why would you remove the _form suffix (also applies to validation and submission callbacks and theming). That is still best practice to include it, right?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -595,46 +595,130 @@ function locale_translate_settings_submit($form, &$form_state) {
+  $form['languages'] = array(
+    '#type' => 'tableselect',
+    '#header' => $header,
...
-  $langcodes = array_filter($form_state['values']['langcodes']);
+  $langcodes = array_filter($form_state['values']['languages']);

I understand the langcodes show up on the UI as languages, but I think it was fine as langcodes. They are language codes after all as this code at the end of my snippet shows. IMHO leave it langcodes.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -595,46 +595,130 @@ function locale_translate_settings_submit($form, &$form_state) {
+  // Check if a language has been selected. tableselect fails to do so.
+  if (!array_filter($form_state['values']['languages'])) {
+     form_set_error('', t('Select a language to update.'));
+  }
+}

The comment is puzzling. Tableselect is not designed to ensure at least one thing is selected, is it? Is that was #required = TRUE on it was supposed to do? That does not work? How other uses of tableselect solve this?

Also minor: the form_set_error() line is indented one space too much.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -656,7 +740,74 @@ function locale_translation_status_form_submit($form, &$form_state) {
+ * Adds labels to the languages and removes checkboxes from languages which have
+ * no updates ane one or more translation files could not be found.

ane?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -656,7 +740,74 @@ function locale_translation_status_form_submit($form, &$form_state) {
+ * Provides debug info when translation update were not found for a project.

"translation *updates* were" or "translation update *was*", no?

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
8.73 KB
31.79 KB

Is this right? ... Is this the current code style for test headers?

According to http://drupal.org/node/1354#files it must be "Contains NameOfTheClass."

What is the significance of this comment? There is nothing happening around it with German?!
German is only added later. Maybe move this there?

Agree, comment modified and moved to where the language is created.

Given this keeps being a form callback, I'm puzzled why would you remove the _form suffix (also applies to validation and submission callbacks and theming). That is still best practice to include it, right?

I can't remember for sure why I did this. Reverted.

I understand the langcodes show up on the UI as languages, but I think it was fine as langcodes. They are language codes after all as this code at the end of my snippet shows. IMHO leave it langcodes.

These are the language codes selected to be updated. Reverted.

The comment is puzzling. Tableselect is not designed to ensure at least one thing is selected, is it? Is that was #required = TRUE on it was supposed to do? That does not work? How other uses of tableselect solve this?

'#required' = TRUE is ignored by tableselect with checkboxes. I guess this is a bug. But I have not found an issue nor documentation on this. Changed the description to be more neutral, but left the '#required' = TRUE to be there when the bug gets fixed.

All other remarks were processes without comment.

Gábor Hojtsy’s picture

Changes look good. Should be RTBC from a code review point of view when it comes back green :)

Status: Needs review » Needs work

The last submitted patch, display-interface-translation-status-1804702-101.patch, failed testing.

Gábor Hojtsy’s picture

Looks like the langcodes fix not fully implemented: "Undefined index: languages in locale.pages.inc on line 711 in locale_translation_status_form_validate()".

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
31.79 KB

Two remaining changes: language -> langcode

Sutharsan’s picture

Now I remember why I used locale_translation_status instead of locale_translation_status_form. Shorter css IDs. Well, lets stick to the standards.
In CSS and JS: locale-translation-status -> locale-translation-status-form

Gábor Hojtsy’s picture

Better to stick to those standards, yeah :)

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Per #102, RTBC. I did a quick code review and tested this and looks really impressive. Thanks for your hard work :)

YesCT’s picture

Here are the tests coding standards:
http://drupal.org/node/325974

We might or might not want to match them.

Sutharsan’s picture

I based the @file documentation change on http://drupal.org/node/1354#files (third example under 'Documenting files'). Note that http://drupal.org/node/325974 is marked 'Needs updating'. If I look through the current use of @file doxygen documentation, many modules use "Definition of NameOfTheClass" (Views, Rest, Breakpoint) some use "Contains NameOfTheClass" (jsonld).

YesCT’s picture

attiks’s picture

This looks good, found a small typo, but can be fixed in a follow up

+++ b/core/modules/locale/locale.installundefined
@@ -283,6 +283,70 @@ function locale_schema() {
+      // Determine the status of the translation updates per lanuage.

typo: language

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks great and very helpful. Committed to 8.x. Thanks.

Sutharsan’s picture

Thanks a lot!! Two month of work have finally payed off :D

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, congrats and thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary took out the to open follow up of changing the no updates to grey/info