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 nameAdd feature to show/hide low priority columns on mobile device, as per module pageUI review (see #63)Accessibility concerns (see #64 and #86)- code review
update status message screenshotsupdate 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.
Status summary on Status report page at /admin/reports/status.
Follow-ups
Need to be opened
- #89 step 6, Q2. 3 of 3 when checking manually.
Unrelated Issues opened
- #1861908: Improve messages when automatically checking for translations
- #1861956: translations imported but error shows: .po could not be copied because the destination directory translations:// is not configur
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
Comments
Comment #1
Gábor HojtsyComment #2
Jose Reyero CreditAttribution: Jose Reyero commentedThis 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.
Comment #3
Sutharsan CreditAttribution: Sutharsan commentedThis is an separate patch to share my code for a translation status ui. It also includes a new form for the translation update settings.
Comment #4
Gábor HojtsySo 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? :)
Comment #5
Gábor HojtsyAdd feature freeze tag.
Comment #6
Gábor HojtsyAlso, 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
Comment #7
Sutharsan CreditAttribution: Sutharsan commentedIt 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.
Comment #9
balintk CreditAttribution: balintk commentedI 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.
Comment #10
Sutharsan CreditAttribution: Sutharsan commentedI 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.
Comment #11
balintk CreditAttribution: balintk commentedAssigning the issue to myself again.
Comment #12
Gábor HojtsyCopy-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.
Comment #13
balintk CreditAttribution: balintk commentedHow 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?
Comment #14
Gábor Hojtsy@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.
Comment #15
Schnitzel CreditAttribution: Schnitzel commentedjust 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.
Comment #16
Jose Reyero CreditAttribution: Jose Reyero commentedWell, 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.
Comment #17
Bojhan CreditAttribution: Bojhan commentedI dont think we need to merge it with modules right now, I also do think we need to determine a MVP option.
Comment #18
Sutharsan CreditAttribution: Sutharsan commentedAgree 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:
My suggestions extending the design:
Comment #19
Sutharsan CreditAttribution: Sutharsan commentedFor 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 ;)
Comment #20
balintk CreditAttribution: balintk commentedI 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:
Outstanding tasks:
I'll try to summarize the todos, please let me know if I missed something.
Comment #21
balintk CreditAttribution: balintk commentedComment #23
Bojhan CreditAttribution: Bojhan commentedThe image is too small to review.
Comment #24
balintk CreditAttribution: balintk commentedSorry, 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.
Comment #25
Gábor HojtsyFixed link to bigger image in #20.
Comment #26
Gábor HojtsyComment #27
Sutharsan CreditAttribution: Sutharsan commented@balintk, Each project-language object can have a 'type' property with these values:
Comment #28
Sutharsan CreditAttribution: Sutharsan commentedYou could differentiate between 'updates' and 'not_found' like this:
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:
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.
Comment #29
Sutharsan CreditAttribution: Sutharsan commentedScreenshot to illustrate available and missing updates:
Comment #30
balintk CreditAttribution: balintk commentedSome progress:
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.
Comment #31
Sutharsan CreditAttribution: Sutharsan commentedRe-rolled against latest patch from #1804688: Download and import interface translations.
Comment #32
Sutharsan CreditAttribution: Sutharsan commentedI have the following suggestions, which I also mocked in the screenshot below.
Comment #33
Sutharsan CreditAttribution: Sutharsan commentedRe-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.
Comment #34
Sutharsan CreditAttribution: Sutharsan commentedOops, forgot to include the js-file.
Comment #35
YesCT CreditAttribution: YesCT commentedI'm testing this and will update with results soon.
Comment #36
YesCT CreditAttribution: YesCT commentedI didn't get done with this testing. Someone else can jump in to do it, or I'll come back to it later today.
Comment #37
balintk CreditAttribution: balintk commentedI 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).
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?
Comment #38
Gábor HojtsyI 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 :)
Comment #39
Bojhan CreditAttribution: Bojhan commentedI 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:
Comment #40
Gábor Hojtsy@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? :)
Comment #41
Bojhan CreditAttribution: Bojhan commented@Gabor Just the toggle > should be the same, all the stylistic parts of the module page you can leave out.
Comment #42
Gábor Hojtsy@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).
Comment #43
Jose Reyero CreditAttribution: Jose Reyero commentedI 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).
Comment #44
Gábor Hojtsy@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).
Comment #44.0
balintk CreditAttribution: balintk commentedUpdated issue summary.
Comment #44.1
balintk CreditAttribution: balintk commentedAdded screenshot to issue summary about user interface changes.
Comment #45
Sutharsan CreditAttribution: Sutharsan commentedRe-rolled against latest patch of #1804688: Download and import interface translations.
Comment #46
Sutharsan CreditAttribution: Sutharsan commentedA 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.
The attached patch includes status messages on the Status report page. Screenshots of the various states are attached for evaluation.
Comment #47
Jose Reyero CreditAttribution: Jose Reyero commentedWell, 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?)
Comment #48
Sutharsan CreditAttribution: Sutharsan commentedA new version with Bojhan's comments in #39 and #41 applied.
Comment #49
Sutharsan CreditAttribution: Sutharsan commentedThe '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.
Comment #50
Bojhan CreditAttribution: Bojhan commentedWe probably should retain the summary, of what modules do have available updates. What about like this?
Ignore the "dutch" styling, mistake.
Comment #51
YesCT CreditAttribution: YesCT commentedI 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."
Comment #52
YesCT CreditAttribution: YesCT commentedListing the actual module names (or core?) in the collapsed summary is even better.
Comment #53
Sutharsan CreditAttribution: Sutharsan commented@YesCT, the screen shot is admin/reports/translations (Available translation updates). The messages are on admin/reports/status (Status report).
Comment #54
Jose Reyero CreditAttribution: Jose Reyero commented@YesCT
+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).
Comment #55
Bojhan CreditAttribution: Bojhan commented@Jose Stop looking only at the images :P I noticed below you should ignore that no checkbox styling it was a mistake.
Comment #56
Sutharsan CreditAttribution: Sutharsan commentedI talked with Bojhan in IRC. He noted that the fist line 'Dutch' was mistake. Disregard that line. Up-to-date languages are not listed.
Comment #57
Gábor Hojtsy+1 to not list up to date languages :)
Comment #58
Sutharsan CreditAttribution: Sutharsan commentedWorking on the patch. I expect to deliver tomorrow.
Comment #58.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated summary
Comment #59
Sutharsan CreditAttribution: Sutharsan commentedAn 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.
Comment #59.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated summary
Comment #59.1
Sutharsan CreditAttribution: Sutharsan commentedUpdated todo list
Comment #60
Sutharsan CreditAttribution: Sutharsan commentedThis 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.
Comment #61
Sutharsan CreditAttribution: Sutharsan commentedOh, yes. It is ready for review.
Comment #63
Gábor Hojtsy@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.
Comment #64
Bojhan CreditAttribution: Bojhan commentedPasses UX Review :) I'm happy with it.
Comment #65
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #65.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #66
Gábor HojtsyOk, adding tag then.
Comment #67
Sutharsan CreditAttribution: Sutharsan commentedIncluding tests, including show/hide low priority columns, improved css.
Comment #67.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #68
Sutharsan CreditAttribution: Sutharsan commentedCorrected patch. To be applied on top of #1804688: Download and import interface translations.
Comment #68.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #69
Gábor HojtsyShould be a testable patch now that #1804688: Download and import interface translations is committed :) Uploaded without the do-not-test suffix. No change.
Comment #71
Sutharsan CreditAttribution: Sutharsan commented#69: display-interface-translation-status-1804702-67.patch queued for re-testing.
Comment #72
Sutharsan CreditAttribution: Sutharsan commentedThe patch applies fine locally and passes tests too. Trying another test run.
Comment #73
Lars Toomre CreditAttribution: Lars Toomre commentedA few observations from reading through the patch in #69:
Should be 'Contains' instead of 'Definition of'. Repeated below in patch.
Missing newline at end of file.
@param directives do not match function variable. Also missing a @return directive.
Missing @param directive.
Missing @param directive.
Comment #74
Gábor Hojtsy@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?
Comment #75
YesCT CreditAttribution: YesCT commentedhttp://drupal.org/coding-standards/docs#themeable
and test coding standards http://drupal.org/node/325974
Comment #75.0
YesCT CreditAttribution: YesCT commentedUpdated issue summray
Comment #76
YesCT CreditAttribution: YesCT commentedUpdated 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).
2. (no patch) admin/reports/translations
3. (no patch) after manual check
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.
5. (patch) expanded
6. (patch) check manually
clicking the check manual, is the same as without the patch (see 3.)
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.
8. (patch with tamper) shows updates available and lists module names
checkbox to update is checked by default. good.
9. (patch with tamper) expanded shows each module with the date of the update, and the projects that do not have updates
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.
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)
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.
Suggestion
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
Comment #77
YesCT CreditAttribution: YesCT commentedTrying to get "all translations up to date"
Looks like there is test coverage for this case, but I was trying to test it manually...
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
I tried disabling block module and tamper to see if I could get a "All translations up to date".
gives in the expanded details:
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:
Comment #78
YesCT CreditAttribution: YesCT commentedI 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
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.
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!
4. it's showing the first enabled core module, but not all of the enabled core modules.
Comment #79
YesCT CreditAttribution: YesCT commentedI 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?
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.
I search in the UI translations screen and limit the search to translated strings and it says zero.
Comment #80
Lars Toomre CreditAttribution: Lars Toomre commentedRe #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.
Comment #81
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #82
YesCT CreditAttribution: YesCT commentedThat 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"
Comment #83
YesCT CreditAttribution: YesCT commented#1861908: Improve messages when automatically checking for translations made for the unrelated 8 of 8 confusion.
Comment #84
YesCT CreditAttribution: YesCT commented#1861956: translations imported but error shows: .po could not be copied because the destination directory translations:// is not configur made for unrelated destination directory error
Comment #85
YesCT CreditAttribution: YesCT commentedI think once we get the new code @Sutharsan mentioned in #81 I'll put up a live site demo to make accessibility reviews easier.
Comment #86
Sutharsan CreditAttribution: Sutharsan commentedOk, 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.
Comment #88
YesCT CreditAttribution: YesCT commented(oops the interdiff was named .patch instead of .txt, so marking needs review since the patch passed the bot)
Comment #89
YesCT CreditAttribution: YesCT commentedUpdated 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.
5. (patch) expanded
Nice. Latest patch fixes this to just say "core"
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.
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.
9. (patch with tamper) expanded shows each module with the date of the update, and the projects that do not have updates
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.
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:
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
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 ?
Comment #90
YesCT CreditAttribution: YesCT commented#1864600: disabling and uninstalling projects messes up interface translation status for problem in #77
Also updating issue summary.
Comment #90.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary with steps to test (previous dependency was committted)
Comment #91
Sutharsan CreditAttribution: Sutharsan commentedI'm fine with changing this to "No updates found" but we could make it a bit stronger: "Untranslated projects" or "Missing translations"
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.
Agree, change to 'for info'/gray.
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%)"
Apparently you have added Spanish. Locale Tamper only mocks modules, no languages.
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.
On the settings page (admin/config/regional/translate/settings) you can choose to include disabled modules too. By default only enabled modules are translated.
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 ..."
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.
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.
Comment #92
YesCT CreditAttribution: YesCT commentedThanks for all those answers! The answer to Q5 made me smile. :)
I'm going to test this again:
me:
@Sutharsan:
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.
Comment #93
YesCT CreditAttribution: YesCT commentedenabled 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
Comment #94
YesCT CreditAttribution: YesCT commentedchanging 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.
Comment #95
YesCT CreditAttribution: YesCT commentedLocated 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
Comment #96
YesCT CreditAttribution: YesCT commentedI 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:
and
Next
Next: final code review and hopefully rtbc.
Comment #96.0
YesCT CreditAttribution: YesCT commentedupdate screen shots, remaining task list, add follow-up/related issues.
Comment #96.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary with recent screenshots
Comment #97
YesCT CreditAttribution: YesCT commentedoops. forgot the patch!
Comment #98
Gábor Hojtsy@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.
Comment #99
Sutharsan CreditAttribution: Sutharsan commentedCreated follow-up issue #1866544: Improve Locale batch progress and result messages and added the code and video link.
Comment #100
Gábor HojtsyI just reviewed the code in this patch. It generally looks pretty good. Mostly minor remarks:
Is this right? I mean we are not testing the test :) Is this the current code style for test headers?
What is the significance of this comment? There is nothing happening around it with German?!
German is only added later. Maybe move this there?
I'd add at the end "and tinker with prepared contributed modules only". Or something along those lines.
.udpate looks like a typo :)
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 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.
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.
ane?
"translation *updates* were" or "translation update *was*", no?
Comment #101
Sutharsan CreditAttribution: Sutharsan commentedAccording to http://drupal.org/node/1354#files it must be "Contains NameOfTheClass."
Agree, comment modified and moved to where the language is created.
I can't remember for sure why I did this. Reverted.
These are the language codes selected to be updated. Reverted.
'#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.
Comment #102
Gábor HojtsyChanges look good. Should be RTBC from a code review point of view when it comes back green :)
Comment #104
Gábor HojtsyLooks like the langcodes fix not fully implemented: "Undefined index: languages in locale.pages.inc on line 711 in locale_translation_status_form_validate()".
Comment #105
Sutharsan CreditAttribution: Sutharsan commentedTwo remaining changes: language -> langcode
Comment #106
Sutharsan CreditAttribution: Sutharsan commentedNow 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
Comment #107
Gábor HojtsyBetter to stick to those standards, yeah :)
Comment #108
penyaskitoPer #102, RTBC. I did a quick code review and tested this and looks really impressive. Thanks for your hard work :)
Comment #109
YesCT CreditAttribution: YesCT commentedHere are the tests coding standards:
http://drupal.org/node/325974
We might or might not want to match them.
Comment #110
Sutharsan CreditAttribution: Sutharsan commentedI 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).
Comment #111
YesCT CreditAttribution: YesCT commentedAgreed. #1869794: Update tests coding standards doc and make consistant with 1354 where appropriate
Comment #112
attiks CreditAttribution: attiks commentedThis looks good, found a small typo, but can be fixed in a follow up
typo: language
Comment #113
Dries CreditAttribution: Dries commentedLooks great and very helpful. Committed to 8.x. Thanks.
Comment #114
Sutharsan CreditAttribution: Sutharsan commentedThanks a lot!! Two month of work have finally payed off :D
Comment #115
Gábor HojtsyWoot, congrats and thanks!
Comment #116.0
(not verified) CreditAttribution: commentedUpdated issue summary took out the to open follow up of changing the no updates to grey/info