Initial virtual folders #3600

unho · 2015-02-23T13:58:52Z

Ready to review.

This is meant to somehow recover our old goals functionality, but using the new vfolders.

This provides a management command to create and update virtual folders, adds the ability to sort the units by (vfolder) priority on the editor, and injects the priority sorting into the translate URLs in the translation project overview.

Things that differ from specs:

We don't use any filtering rules. Instead we provide lists of filenames relative to vfolder location.

julen · 2015-02-24T08:05:16Z

pootle/core/browser.py

-    item = make_generic_item(directory)
+    filters = {}
+
+    if len(VirtualFolder.get_matching_for(directory.pootle_path)):


As I see this will incur in N*M+1 queries, where:

N is the number of directories being displayed in the overview,

M is the number of filtering rules in each browsable VirtualFolder, and

1 is the query needed for retrieving browsable VirtualFolders

Looks excessive on a first sight, even more taking into account there's no caching of any sort.

This has been replaced by a new call that triggers only 1 query.

Regarding the cache, the plan is to get something that works, and then optimize it using cache.

unho · 2015-03-02T09:17:43Z

In order to check this locally you will have to follow these steps:

Check out the branch locally
Ensure you have a project with several files on it (per language). It is recommended to have enough files to include some of them in a vfolder while leaving some of them out
Run migrations
Craft a JSON file to add some sample vfolders. Example in unho@5dfcc14#diff-066beddb177d42215271216483cd3cb8 It is recommended to have a directory with 4 files at least, and include 3 of them on vfolders (one vfolder with 3 files, and the other two vfolders with 2 files, sharing only one of them).
Run add_vfolders management command to import such vfolders. Example in unho@5dfcc14#diff-32d9c83230475a8ff37b1695b1606581R372
Run the server and go to a path where we know there is some of the files matched by a vfolder
Check that the translate links above the stats table have the sort=priority bit on the URLs
Check that the translate links on the stats table for the files or directories with files included in a vfolder also include that sort=priority bit on the URLs
Click on any of those translate links and ensure the units are sorted by priority (since we are currently including the units on vfolders using the stores, we just need to ensure that the vfolders are in the appropriate order).

julen · 2015-03-02T10:56:47Z

docs/features/virtual_folders.rst

+.. versionadded:: 2.7
+
+Virtual folders provide a way to group translations based on any criteria,
+including a same file across all the languages in a project, or specific files.


What does a same file mean here? Can you maybe reword it?

It means that a file named file.po is matched in all languages in a project. For example /af/proj/file.po or /eu/proj/file.po.

Rephrased to avoid confusion.

julen · 2015-03-04T08:39:35Z

docs/features/virtual_folders.rst

+.. _virtual_folders#attributes:
+
+Virtual folders attributes
+-----------------------------------


Underline unnecessarily long.

iafan · 2015-03-16T23:31:48Z

Thanks, @julen. Are you able to setup v-folders on your side? I think I tried to closely follow the sample JSON provided, but have no luck seeing the v-folders in the browser

iafan · 2015-03-16T23:34:51Z

@julen,

I can actaully see the empty but open side panel if I go to https://translate.stage.evernote.com/ru/website3_evernote/home/ (again, this might be some side effect of caching, but worth checking on your side)

dwaynebailey · 2015-03-17T01:57:37Z

@iafan this first stage commit has no UI. The change forces editing based on v-folder priority. The second stage, not yet PR'd, adds the UI for the TP level.

iafan · 2015-03-17T02:28:00Z

@dwaynebailey, so how one is supposed to test this? It makes sense to add UI to this PR as well, and test the feature as a whole, IMO.

julen · 2015-03-17T07:07:53Z

@julen,

I can actaully see the empty but open side panel if I go to https://translate.stage.evernote.com/ru/website3_evernote/home/ (again, this might be some side effect of caching, but worth checking on your side)

@iafan I realize some setting changes to disable the imports/exports were uncommitted on myside, and that's fixed now. In any case this displays that imports/exports in master is not in good shape, and I already reported this on IRC.

unho · 2015-03-17T08:42:15Z

pootle/apps/virtualfolder/models.py

+            return locations
+        elif "{LANG}" in self.location:
+            return [self.location.replace("{LANG}", lang.code)
+                    for lang in Language.objects.all()]


This should use only current project languages.

unho · 2015-03-17T09:02:28Z

It seems that I missed the party.

I did a first round of review of this PR on our staging server, which wasn't quite successful. In short, I was unable to successfully add any v-folder description that would show up in the UI. I don't know if this is related to caching or not, but nothing changed from the UI standpoint when I added v-folders via add_vfolders command. I tried simplest scenarios when there's a v-folder inside a folder A, which combines two files (B.po and C.po) located in the same folder A.

No descriptions are being displayed as it is not yet clear to me where does should be displayed.

Also, the current v-folder functionality is very limited (my assumptions here are based on the documentation and samples alone): one can only provide the set of files to group together. In our case, we can only imagine real use of this feature when one can specify any arbitrary folder (starting with project folders).

This is just the first step. In the future you can expand this as necessary to support more filtering methods. For now using stores just recovers our lost goals functionality.

It is not obvious how to delete v-folders. When I delete vfolder definitions from the .json file and run add_vfolders, nothing happens. Does add_vfolders (or any other command) support deletion at all?

No. This is not yet implemented.

You can instead hide vfolders by setting them to not being browsable. Changing vfolders is supported through the add_vfolders command by just providing a JSON file altered accordingly.

It is not obvious whether one needs to include leading and/or trailing slash in location path. The command gives no errors.

I was assuming that people would check the docs and the sample JSON in case of doubt. As you can see the sample JSON always includes the trailing slashes. Just checked the code and it never complains, but in some cases the absence of trailing slash might result on not getting the desired results on filtering.

Side notes: I noticed these regressions:

the sidebar is always visible (can be expanded/collapsed), even it doesn't have any content in it. In the master branch code, it is not available if it has nothing to display.

the sidebar height depends on the size of the content (the number of rows in the browsing table). It should take the full height of the content area.

No change is done in this PR to the sidebar, so these should be issues already present on master.

To sum it up, this definitely needs another round of testing and bugfixing before the feature goes live.

I don't think this PR has bugs. Re testing, see below.

@dwaynebailey, so how one is supposed to test this? It makes sense to add UI to this PR as well, and test the feature as a whole, IMO.

Please see the testing steps provided on comment #3600 (comment) I am aware that lack of UI difficults too much the testing, but the steps in that comment should be enough. If they are not enough we can talk about appending here some of the commits already finished that add UI bits, despite that I would prefer to land the current set of commits since has been tested and reworked several times instead of adding more commits that might need to be reworked more times thus delaying this PR even more.

unho · 2015-03-17T09:28:29Z

@iafan I agree we need a way to remove a vfolder.

IMHO considering that a vfolder that is not on the JSON means that it should be removed is a bad idea. I mean that if you have 100 vfolders you usually going to use the JSON to just alter some vfolders (lets say 5 of them) and having to deal with a JSON with 100 vfolder is not comfortable. Besides that having a add_vfolders that allows to remove them is kind of confusing.

Another option is to add a new management command del_vfolder which accepts some data identifying a vfolder and removes it. I think it is better for it to remove only a single vfolder at a time. Since vfolders have no unique identifiers (languages and projects have their codes) we will have to provide a name and a location, so we might end with something like:

$ ./manage.py del_vfolder my-vfolder /{LANG}/proj/dir/subdir/subsubdir/

which I am not sure if can be considered simple and easy.

Anyway whichever way we take re deleting, IMHO it is less priority than providing a working UI for translators to work with vfolders. Despite it is an ugly workaround, you can always hide vfolders by setting them to not being browsable.

dwaynebailey · 2015-03-17T10:36:39Z

@unho can you create an issue to track the del_vfolder command. I think we can delay it with the 1.0 and not browesable hack. But we do need to fix it to complete the feature.

dwaynebailey · 2015-03-17T10:39:27Z

@iafan the way I tested was as following. I created 3 simple 3 unit PO files (translated, untranslated, fuzzy) a.po, b.po, c.po. I created vfolder that set 2.0 for b.po and 0.5 priorities for a.po. Used the editor to ensure that the files appear in the order specified which would be b.po, c.po, a.po.

This first implementation is designed simply to ensure the backend for storing goals is good and that priority sorting of gaols works correctly.

unho · 2015-03-17T11:13:28Z

@dwaynebailey Done: #3634

Using a m2m field between VirtualFolder and Unit will ease fast retrieving of matching units and vfolders, also will allow to retrieve units sorted by vfolder priority. The priority field is a float in order to be able to set priorities between 0 and 1. Validators are not run on instance save, so the we can't use validators to ensure the priority is greater than zero. Part of translate#3521.

Fixes translate#3521.

Fixes translate#3613.

Not browsable vfolders are not returned. Fixes translate#3522.

The 'view all' link is never sorted. Part of translate#3614.

Links to all units are not sorted. Also this is only for directories, since we are currently recovering our previous goals functionality, which used stores. Fixes translate#3614.

iafan · 2015-03-17T20:46:59Z

This first implementation is designed simply to ensure the backend for storing goals is good and that priority sorting of gaols works correctly.

Then I can delegate the review to @julen. If this feature is not visible and has not reached the level it can be of real use to us (Evernote), I'm ok to have it merged if Julen confirms that it looks sane from the backend side. I'll be happy to review this once the UI part has landed, and once we can start testing this on some real-life scenarios (for us the first most obvious case will be to group projects together, which needs the filters to support paths, not individual .po files).

iafan · 2015-03-17T20:54:13Z

IMHO considering that a vfolder that is not on the JSON means that it should be removed is a bad idea. I mean that if you have 100 vfolders you usually going to use the JSON to just alter some vfolders (lets say 5 of them) and having to deal with a JSON with 100 vfolder is not comfortable. Besides that having a add_vfolders that allows to remove them is kind of confusing.

@unho, I think I disagree on this. From my perspective, it would be much easier to have one JSON file which fully represents the config for all v-folders, and maintain it, adding or deleting entries from it. Then a single command, say, update_vfolders would synchronize this JSON with the database, creating, updating or deleting v-folders accordingly.

Otherwise, you're going to have two commands (add_vfolders and del_vfolders) which you have to remember about and maintain data for, and deletion of vfolders will be a much less intuitive task than just deleting relevant entries from a single JSON.

unho · 2015-03-17T21:11:28Z

IMHO considering that a vfolder that is not on the JSON means that it should be removed is a bad idea. I mean that if you have 100 vfolders you usually going to use the JSON to just alter some vfolders (lets say 5 of them) and having to deal with a JSON with 100 vfolder is not comfortable. Besides that having a add_vfolders that allows to remove them is kind of confusing.

@unho, I think I disagree on this. From my perspective, it would be much easier to have one JSON file which fully represents the config for all v-folders, and maintain it, adding or deleting entries from it. Then a single command, say, update_vfolders would synchronize this JSON with the database, creating, updating or deleting v-folders accordingly.

Otherwise, you're going to have two commands (add_vfolders and del_vfolders) which you have to remember about and maintain data for, and deletion of vfolders will be a much less intuitive task than just deleting relevant entries from a single JSON.

I see your point, but I was thinking on a several admins scenario. Maybe yours is better.

@dwaynebailey any input on this?

(for us the first most obvious case will be to group projects together, which needs the filters to support paths, not individual .po files).

@iafan Can you explain this a bit more?

iafan · 2015-03-17T21:30:44Z

I see your point, but I was thinking on a several admins scenario.

I think this is pretty similar to regular Pootle configs. We have several Pootle admins, and they have an access to the Pootle configuration file, which we keep in Git. The same thing is applicable to this JSON config — especially if there are multiple people working with it — it makes sense to keep it in version control (and this is where its history will be stored), and then apply it as a whole (e.g. as a part of deployment script) to the server.

iafan · 2015-03-17T21:39:52Z

(for us the first most obvious case will be to group projects together, which needs the filters to support paths, not individual .po files).
@iafan Can you explain this a bit more?

This is pretty straightforward: imagine you can set up any path prefix in your filters > files section of the v-folder JSON entry. This way you would include all files down that tree, and this should work right from any point in the tree. Consider this example:

    {
        "name": "ios_skitch_evernote_bundle",
        "location": "/{LANG}/",
        "filters": {
            "files": [
                "ios_skitch_evernote",
                "common_skitch_evernote"
            ]
        }
    },

This is a v-folder that lives inside each language folder, and which groups two translation projects together, identified by their relative paths: ios_skitch_evernote and common_skitch_evernote.

unho · 2015-03-18T08:53:13Z

@iafan Implementing filtering by path besides filtering by specific filenames looks easy to implement. Added issue #3643 to track it.

unho · 2015-03-18T10:15:29Z

@iafan Lets move discussion re deleting vfolders to issue #3634.

unho changed the title ~~Recovering goals~~ Initial virtual folders Feb 23, 2015

unho self-assigned this Feb 23, 2015

julen reviewed Feb 24, 2015
View reviewed changes

unho force-pushed the recovering-goals branch from 6adea80 to bfe72eb Compare February 27, 2015 21:49

julen reviewed Mar 2, 2015
View reviewed changes

unho force-pushed the recovering-goals branch 4 times, most recently from 2e4fbe1 to 5e6d829 Compare March 3, 2015 19:55

julen reviewed Mar 4, 2015
View reviewed changes

unho force-pushed the recovering-goals branch 2 times, most recently from c48830e to 853893a Compare March 5, 2015 09:40

unho reviewed Mar 17, 2015
View reviewed changes

unho force-pushed the recovering-goals branch from 2f2c385 to 4e9ad2f Compare March 17, 2015 11:39

unho added 6 commits March 17, 2015 15:27

VFolder: Add management command to bulk import vfolders

d1b1fe9

Fixes translate#3521.

Editor: Sort strings by priority

f106dd1

Fixes translate#3613.

VFolder: Retrieve vfolders to display in a given path

fa8696e

Not browsable vfolders are not returned. Fixes translate#3522.

Overview: Provide links to sort by priority on editor

6e89134

The 'view all' link is never sorted. Part of translate#3614.

Overview: Provide sorted translate links when a vfolder applies

b3ea3ee

Links to all units are not sorted. Also this is only for directories, since we are currently recovering our previous goals functionality, which used stores. Fixes translate#3614.

unho force-pushed the recovering-goals branch from 4e9ad2f to b3ea3ee Compare March 17, 2015 14:28

unho merged commit b3ea3ee into translate:master Mar 17, 2015

unho mentioned this pull request Mar 18, 2015

VFolders: Add management command to delete vfolders #3634

Open

jleclanche mentioned this pull request Apr 8, 2015

Get goals back in core #3301

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial virtual folders #3600

Initial virtual folders #3600

unho commented Feb 23, 2015

julen Feb 24, 2015

unho Feb 26, 2015

unho commented Mar 2, 2015

julen Mar 2, 2015

unho Mar 2, 2015

julen Mar 4, 2015

unho Mar 4, 2015

iafan commented Mar 16, 2015

iafan commented Mar 16, 2015

dwaynebailey commented Mar 17, 2015

iafan commented Mar 17, 2015

julen commented Mar 17, 2015

unho Mar 17, 2015

unho commented Mar 17, 2015

unho commented Mar 17, 2015

dwaynebailey commented Mar 17, 2015

dwaynebailey commented Mar 17, 2015

unho commented Mar 17, 2015

iafan commented Mar 17, 2015

iafan commented Mar 17, 2015

unho commented Mar 17, 2015

iafan commented Mar 17, 2015

iafan commented Mar 17, 2015

unho commented Mar 18, 2015

unho commented Mar 18, 2015

Initial virtual folders #3600

Initial virtual folders #3600

Conversation

unho commented Feb 23, 2015

julen Feb 24, 2015

Choose a reason for hiding this comment

unho Feb 26, 2015

Choose a reason for hiding this comment

unho commented Mar 2, 2015

julen Mar 2, 2015

Choose a reason for hiding this comment

unho Mar 2, 2015

Choose a reason for hiding this comment

julen Mar 4, 2015

Choose a reason for hiding this comment

unho Mar 4, 2015

Choose a reason for hiding this comment

iafan commented Mar 16, 2015

iafan commented Mar 16, 2015

dwaynebailey commented Mar 17, 2015

iafan commented Mar 17, 2015

julen commented Mar 17, 2015

unho Mar 17, 2015

Choose a reason for hiding this comment

unho commented Mar 17, 2015

unho commented Mar 17, 2015

dwaynebailey commented Mar 17, 2015

dwaynebailey commented Mar 17, 2015

unho commented Mar 17, 2015

iafan commented Mar 17, 2015

iafan commented Mar 17, 2015

unho commented Mar 17, 2015

iafan commented Mar 17, 2015

iafan commented Mar 17, 2015

unho commented Mar 18, 2015

unho commented Mar 18, 2015