Bring quality checks documentation within Pootle #3684

unho · 2015-04-22T15:26:58Z

This staticpage also have to be created on the upgrade to 2.6. Working on that.

@translate/evernote @translate/house Please review.

Ideally Evernote checks would use the same approach it is used for TTK checks: retrieve description from each check docstring.

Also note that this doesn't handle the scenario where TTK is upgraded and new checks are added, or removed or changed.

dwaynebailey · 2015-04-22T16:08:42Z

lgtm didn't test. I'm not sure about the unescaped & that appear in the reference EN HTML file.

iafan · 2015-04-22T16:38:29Z

Please note that we added some new quality checks and descriptions recently (couple of weeks ago). Is this based on most recent edits? After this PR is merged, who's going to maintain this file?

iafan · 2015-04-22T16:40:06Z

Yes. It could be put as & but works the same.

My personal preference to have all HTML files actually XHTML (so that they can be parsed/validated as a part of the test/build process). So I'd suggest using & everywhere (including URLs).

unho · 2015-04-22T18:49:45Z

@iafan All Evernote quality checks have description except https://github.com/translate/pootle/blob/master/pootle/apps/pootle_misc/checks.py#L455-L463 which doesn't look like a quality check. Or at least it seems not to be used anywhere.

About maintenance. I suppose it is up to the people that adds or removes checks. IMHO it might be better to put each quality check description in the docstring, like we just did with TTK ones: translate/translate@602b9286

I am replacing the &.

julen · 2015-04-23T06:26:51Z

pootle/core/initdb.py

@@ -268,7 +274,7 @@ def create_default_projects():
                 'guide</a>.</div>'),
        'virtual_path': "announcements/projects/"+tutorial.code,
    }
-    ann = Announcement(**criteria)
+    ann = StaticPage(**criteria)


Is this an error?

Hmm just saw the change in the import. Still, I don't believe it's necessary the rename, and it's completely irrelevant in the context of this PR.

When I first imported StaticPage was for using it on announcement creation, so I imported it as Announcement. Now that we are creating regular static pages it feels cleaner to use instead StaticPage since using Announcement can be confusing.

julen · 2015-04-23T06:49:35Z

Not sure how the requirements/base.txt changes are relevant here.
Can we have the HTML file somewhere else please? templates/checks/descriptions.html is a good candidate.
Also I wonder how are descriptions supposed to be updated whenever new checks appear or disappear.

unho · 2015-04-23T11:31:26Z

Not sure how the requirements/base.txt changes are relevant here.

We require latest available Toolkit code, because of the changes we recently merged there (each check function now keeps its description on its docstring for easier maintenance and to ease programmatic regeneration), so I would say it is necessary.

Can we have the HTML file somewhere else please? templates/checks/descriptions.html is a good candidate.

I don't like the idea, but I changed it that way.

In fact I think the best approach to keep the descriptions updated would be to move them to each quality check docstring (like in TTK), but since that would take time I would leave it for future cleanups. Also I am not sure if you are even ok with this approach.

Also I wonder how are descriptions supposed to be updated whenever new checks appear or disappear.

There is no way to do that in this PR. Any suggestion? Maybe some management command to regenerate the whole checks descriptions staticpage?

dwaynebailey · 2015-04-24T10:01:31Z

@unho a management command does sound like the easiest to recreate the pages, buys us time to not have to solve the further for a bit of time.

unho · 2015-04-24T18:10:27Z

@translate/evernote @translate/house Added a missing requirement and a new management command to recreate the checks descriptions on demand (docs included).

julen · 2015-04-27T15:57:49Z

The new dependency on docutils for the base requirements made me think if the docstring extraction + rst2html conversion is not something better worth doing as a external/build step. After all check docstrings rarely change, and this step (and the dependency on docutils) can easily be avoided for the rest of humans. So initdb would just have to read a pre-generated HTML file and import it as an static page.

unho · 2015-04-27T19:45:35Z

@julen So you suggest to include a static HTML page that has to be recreated on each release? Do you suggest us to handle the descriptions for the TTK checks in the same way?

unho · 2015-04-27T19:54:34Z

@julen Let me try to explain my point a bit more clearly. Having to maintain static HTML files both in Pootle and TTK to avoid having a new dependency feels like a high toll. I see that as a maintenance nightmare, to be clear. In fact I was looking more towards even removing the existing HTML file in Pootle for the Evernote checks description, specially after @iafan asked about who is going to maintain that file (which I understood as that he sees that as maintenance burden). On the other hand I don't like the idea of pulling new dependencies.

iafan · 2015-04-27T19:56:15Z

I personally don't quite like what @julen have proposed: if we change the description in the bundled html file, it won't get propagated to static files in the database. We'd better simply serve this as a static page (and outside the /pages/ prefix) without having to copy it anywhere.

julen · 2015-04-27T20:01:39Z

I personally don't quite like what @julen have proposed: if we change the description in the bundled html file, it won't get propagated to static files in the database.

This is not any new proposal, but how the code in the PR operates.

We'd better simply serve this as a static page (and outside the /pages/ prefix) without having to copy it anywhere.

Agreed, this would avoid the maintenance cost on creating a StaticPage, but as you see there's a build process (rst2html) as the TTK descriptions are pulled from the Python source directly.

iafan · 2015-04-27T20:13:02Z

Ok, I was probably confused by this comment of yours:

So initdb would just have to read a pre-generated HTML file and import it as an static page.

This is what I would like to avoid. We don't have to copy template into a 'static page' entity in the database just to be able to show static content. We can render it straight from the template.

julen · 2015-04-27T20:13:35Z

Having to maintain static HTML files both in Pootle and TTK to avoid having a new dependency feels like a high toll.

Forbid me if my comment wasn't clear enough, but I'm not suggesting to maintain any HTML files on TTK. The explanation/help page is on the Pootle side, so any HTML file belong there.

Also, here a static HTML string is being generated, which is then fed to an StaticPage object. Note that at the current state of things generating the output HTML string is necessary in any case, but creating the StaticPage object adds an extra layer which can be avoided by serving the HTML directly.

dwaynebailey · 2015-04-29T08:59:23Z

@julen @iafan OK let me weigh in here. Firstly, this PR was originally done to try quickly get TTK check descriptions into Pootle and also EN check descriptions into a default install. Secondly, to make it possible to keep those well maintained without duplication. So lets keep that in mind.

I also think there is a better way to support check descriptions and that's close to the check and not in a static page. But we didn't want to go that route for this, at that would be a long fix... which ironically this quick fix is now turning into.

@julen from what I understand your issues and suggestions are these:

We don't actually need a Static Page object we could just have a static HTML page (I think @iafan agrees) that Pootle is serving.
We can remove the docutils dependency by simply creating the HTML file as part of the Pootle release process, instead of the build process. header+encheck+ttkchecks = static HTML file

@unho raises the following concern with this approach:

TTK can be anything and thus any new TTK release would require you to generate the static page again to pick up any new checks or changed descriptions. Thus there is no one static page for TTK

My suggestion for going forward:

Drop StaticPage
Generate the TTK part as part of the Pootle release process
Keep the management command to allow regeneration of the static HTML
Move docutils dependency into dev.txt. Thus anyone wanting to update the checks page can do it.

julen · 2015-04-29T11:51:15Z

Your understanding is correct @dwaynebailey and the suggestion is reasonable.

Also note the current StaticPage is available at /pages/help/quality-checks/. @iafan was suggesting to keep the new HTML file out of the /pages/ namespace, although it would be possible to retain the old URL if deemed necessary. Thoughts @iafan?

unho · 2015-04-29T11:56:29Z

@julen @iafan I see no problem on having the descriptions on a static HTML that is served directly. But note that that requires to adjust the links on the editor and to agree in which URL is the static HTML going to be served.

@dwaynebailey Keeping the command to regenerate the descriptions (necessary since a given Pootle version can support multiple TTK releases with possibly different checks) pulls in the docutils dependency. If that dependency is not in base requirements we will have to warn people on docs about the need to install an extra dependency and enclose the import on a try block to avoid tracebacks in case of docutils not being available. And that is fine, despite IMHO it is not precisely user friendly.

All of this just reminded me that we have to include checks description regeneration in the upgrade instructions.

unho · 2015-05-03T20:20:27Z

@translate/evernote @translate/house Reworked the whole PR. Now quality checks descriptions are being served in /help/quality-checks/ (so no more static pages). The management command now only regenerates the Translate Toolkit checks descriptions, which are saved in the checks/_ttk_descriptions.html template. Also added changes in docs, kept the requirement for the newer Toolkit and code is failsafe for docutils absence.

julen · 2015-05-04T07:16:40Z

pootle/apps/pootle_app/management/commands/regenerate_checks_descriptions.py

+        body = u"\n".join(filterdocs)
+
+        # Output the quality checks descriptions to the HTML file.
+        filename = os.path.join(


Better:

from django.conf import settings filename = os.path.join(settings.WORKING_DIR, 'templates/checks/_ttk_descriptions.html')

julen · 2015-05-04T07:35:06Z

Thanks @unho, this looks good now.

I don't believe an empty _ttk_descriptions.html needs to be under version control, and even more, it should probably be in the .gitignore file as it should be generated when creating a distribution package.

You might as well want to align file names with URLs (checks/descriptions.html -> help/quality_checks.html), but up to you.

The only concern I have is that when the command hasn't been run (or when there's no plan to use TTK checks), it still displays an empty header (Translate Toolkit Checks) and a paragraph mentioning two types of checks.
From our side we can customize that template to remove these irrelevant references and be set with that; not sure about other potential scenarios though. @iafan your thoughts on this?

GTM from my side but let's wait to hear thoughts on the previous point before merging.

unho · 2015-05-04T10:30:45Z

I don't believe an empty _ttk_descriptions.html needs to be under version control, and even more, it should probably be in the .gitignore file as it should be generated when creating a distribution package.

This template now contains a placeholder text saying TTK checks are not enabled.

You might as well want to align file names with URLs (checks/descriptions.html -> help/quality_checks.html), but up to you.

Done.

The only concern I have is that when the command hasn't been run (or when there's no plan to use TTK checks), it still displays an empty header (Translate Toolkit Checks) and a paragraph mentioning two types of checks.
From our side we can customize that template to remove these irrelevant references and be set with that; not sure about other potential scenarios though. @iafan your thoughts on this?

Now the Translate Toolkit Checks part displays a placeholder saying that those are not enabled. I think this approach might not be clean enough, and I have been thinking on having a third include for the problematic paragraph while having the TTK include blank. But I would like to hear from you.

julen · 2015-05-05T21:23:46Z

pootle/apps/pootle_app/management/commands/regenerate_checks_descriptions.py

+        with codecs.open(filename, 'w', 'utf-8') as f:
+            f.write(body)
+
+        self.stdout.write('Regenerating descriptions... done.')


Realize you're repeating Regenerating descriptions here, it's already written above.

@julen It is intentional. Lets not force users to guess what is done.

I'd understand it if it was displayed as Regenerating descriptions... at the beginning, and once it's done it was overwritten with Regenerating descriptions... done. There's no guesswork involved.

@julen Ok, changing this so you can gtm.

julen · 2015-05-05T21:30:32Z

I don't believe an empty _ttk_descriptions.html needs to be under version control, and even more, it should probably be in the .gitignore file as it should be generated when creating a distribution package.

This template now contains a placeholder text saying TTK checks are not enabled.

The core issue I was trying to highlight wasn't the fact of having an empty file. It was about the fact of having a target file which can be generated and therefore possibly end up accidentally modified in version control, potentially creating conflicts or causing other trouble that can initially be avoided.

dwaynebailey · 2015-05-05T23:52:45Z

pootle/templates/help/_ttk_quality_checks.html

+<h2>Translate Toolkit Checks</h2>
+
+
+<p>This server has not enabled the Translate Toolkit quality checks.</p>


Why have this at all if the server is not using TTK checks?

dwaynebailey · 2015-05-06T00:07:54Z

@unho did a review and the generation works.

One concern. The docs that we're replacing in TTK have full description of checks, much like Evernote. Yet in the autogenerated ones we only have the first line of the content.

Is this a bug, the code looks like it wants to give the full description. Or was it intentional?

unho · 2015-05-06T07:38:46Z

One concern. The docs that we're replacing in TTK have full description of checks, much like Evernote. Yet in the autogenerated ones we only have the first line of the content.

Is this a bug, the code looks like it wants to give the full description. Or was it intentional?

@dwaynebailey That might be you not having upgraded translate-toolkit dependency. Note that we require the latest version on git. Once we release 1.13 final we will require that.

dwaynebailey · 2015-05-06T10:18:00Z

@unho spot on it was an older toolkit instead of the one from Git. With a new version from github this works as expected.

unho · 2015-05-06T11:03:07Z

@translate/house @translate/evernote All issues have been addressed. Now the help/_ttk_quality_checks.html template is not being added and is on the gitignore. Also all the references to toolkit or Evernote have been removed and the CSS styles are now in the stylesheets. Any other issue or gtms?

julen · 2015-05-06T11:14:42Z

pootle/templates/help/quality_checks.html

+
+  <p>Pootle has an ability to check for common translation mistakes. Once you submit a translation, it will compare its certain features with the original string and identify potential problems. Сhecks are displayed in the upper right corner, just above the submit button. Once there are checks in red, you will stay on the same unit, until you have each check resolved or muted. Note that you should only mute checks if you know what you're doing. Muted checks will be reviewed periodically by the managers.</p>
+
+  <p>If you are not sure what a particular error means and how to fix it properly, use <i>"Report a problem with this string"</i> link at the top of the unit. The managers will try to assist you as soon as possible.</p>


Realize this only applies if CAN_CONTACT = True

Fixed that, despite the string errors report form is displayed indepedently of the value of CAN_CONTACT.

This allows us to have a specific URL pattern to refer to them. Note that the help/_ttk_quality_checks.html template is not added since it is intended to be automatically generated. Part of translate#3604.

unho · 2015-05-06T14:06:20Z

@translate/house @translate/evernote Any new issue?

dwaynebailey · 2015-05-06T16:08:36Z

@unho lgtm

The resulting descriptions are saved as the help/_ttk_quality_checks.html template. That filepath is added to ignore in order to avoid inadvertently commits changes in this template. This requires the latest available code of Translate Toolkit in order to work as expected. The docutils library is also required. If not present an error message is displayed asking for it. Fixes translate#3604.

unho · 2015-05-06T20:15:10Z

Merged since it seems I have gtms from both sides. If any other issue arises it can be fixed on master.

This reverts commit 6008d34. Here's the comment in the original PR about why it shouldn't be there #3684 (comment)

unho force-pushed the bring-checks-description-to-pootle branch from 0102100 to 7e840ca Compare April 22, 2015 18:53

unho mentioned this pull request Apr 22, 2015

Upgrade: Bring quality checks documentation within Pootle #3685

Merged

unho force-pushed the bring-checks-description-to-pootle branch 2 times, most recently from cc9004b to 0f399d8 Compare April 22, 2015 19:43

julen reviewed Apr 23, 2015
View reviewed changes

unho force-pushed the bring-checks-description-to-pootle branch 2 times, most recently from 91aeb6d to 49415ea Compare April 23, 2015 11:28

unho force-pushed the bring-checks-description-to-pootle branch from 49415ea to 2f811c2 Compare April 24, 2015 17:58

unho force-pushed the bring-checks-description-to-pootle branch 2 times, most recently from 2642ef2 to fe9b5f5 Compare April 27, 2015 08:08

unho force-pushed the bring-checks-description-to-pootle branch from fe9b5f5 to a962374 Compare April 28, 2015 21:06

julen reviewed May 4, 2015
View reviewed changes

unho force-pushed the bring-checks-description-to-pootle branch from 53983b6 to 73017e6 Compare May 4, 2015 10:24

unho force-pushed the bring-checks-description-to-pootle branch 2 times, most recently from 1e1957f to 79e5310 Compare May 5, 2015 21:18

julen reviewed May 5, 2015
View reviewed changes

dwaynebailey reviewed May 5, 2015
View reviewed changes

unho force-pushed the bring-checks-description-to-pootle branch from 79e5310 to 65ac1ff Compare May 6, 2015 07:35

unho force-pushed the bring-checks-description-to-pootle branch from 65ac1ff to 784f1bc Compare May 6, 2015 11:00

unho force-pushed the bring-checks-description-to-pootle branch from 784f1bc to 5e2af4b Compare May 6, 2015 11:07

julen reviewed May 6, 2015
View reviewed changes

unho force-pushed the bring-checks-description-to-pootle branch from 5e2af4b to f3bd6a0 Compare May 6, 2015 11:19

Serve checks descriptions as new view

4be2277

This allows us to have a specific URL pattern to refer to them. Note that the help/_ttk_quality_checks.html template is not added since it is intended to be automatically generated. Part of translate#3604.

unho force-pushed the bring-checks-description-to-pootle branch from f3bd6a0 to 991e541 Compare May 6, 2015 11:58

unho force-pushed the bring-checks-description-to-pootle branch from 991e541 to e63082c Compare May 6, 2015 20:10

unho merged commit e63082c into translate:master May 6, 2015

unho deleted the bring-checks-description-to-pootle branch May 6, 2015 20:13

dwaynebailey added a commit that referenced this pull request May 22, 2015

Revert "Add missing template"

d8b4de9

This reverts commit 6008d34. Here's the comment in the original PR about why it shouldn't be there #3684 (comment)

		<h2>Translate Toolkit Checks</h2>


		<p>This server has not enabled the Translate Toolkit quality checks.</p>


		<p>Pootle has an ability to check for common translation mistakes. Once you submit a translation, it will compare its certain features with the original string and identify potential problems. Сhecks are displayed in the upper right corner, just above the submit button. Once there are checks in red, you will stay on the same unit, until you have each check resolved or muted. Note that you should only mute checks if you know what you're doing. Muted checks will be reviewed periodically by the managers.</p>

		<p>If you are not sure what a particular error means and how to fix it properly, use <i>"Report a problem with this string"</i> link at the top of the unit. The managers will try to assist you as soon as possible.</p>

Bring quality checks documentation within Pootle #3684

Bring quality checks documentation within Pootle #3684

Conversation

unho commented Apr 22, 2015

dwaynebailey commented Apr 22, 2015

iafan commented Apr 22, 2015

iafan commented Apr 22, 2015

unho commented Apr 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julen commented Apr 23, 2015

unho commented Apr 23, 2015

dwaynebailey commented Apr 24, 2015

unho commented Apr 24, 2015

julen commented Apr 27, 2015

unho commented Apr 27, 2015

unho commented Apr 27, 2015

iafan commented Apr 27, 2015

julen commented Apr 27, 2015

iafan commented Apr 27, 2015

julen commented Apr 27, 2015

dwaynebailey commented Apr 29, 2015

julen commented Apr 29, 2015

unho commented Apr 29, 2015

unho commented May 3, 2015

Choose a reason for hiding this comment

julen commented May 4, 2015

unho commented May 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julen commented May 5, 2015

Choose a reason for hiding this comment

dwaynebailey commented May 6, 2015

unho commented May 6, 2015

dwaynebailey commented May 6, 2015

unho commented May 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unho commented May 6, 2015

dwaynebailey commented May 6, 2015

unho commented May 6, 2015