TAPevents / tap-i18n-db

MIT License
51 stars 17 forks source link

Support removing specific translations #2

Open rijk opened 10 years ago

rijk commented 10 years ago

I have run into a scenario where the removeTranslation() and removeLanguage() functionality is not sufficient. In my collection I'm storing options to a multiple choice question:

{
    "_id" : "FMMepvz85Ne9CvE3L",
    "title" : "Multiple Choice",
    "correct" : 5,
    "options" : [ 
        {
            "text" : "option one",
            "value" : 2
        }, 
        {
            "text" : "option two",
            "value" : 5
        }, 
        {
            "text" : "option three",
            "value" : 3
        }
    ],
    "i18n" : {
        "nl" : {
            "options" : [ 
                {
                    "text" : "optie een",
                    "value" : 2
                }, 
                {
                    "text" : "optie twee",
                    "value" : 5
                }, 
                {
                    "text" : "optie drie",
                    "value" : 3
                }
            ],
            "title" : "Meerkeuze"
        }
    }
}

This works perfectly. When removing an option though, it is very important that the translation is also removed. Otherwise, the interface may display deleted options, or incorrect translations based on the array key.

As the remove*() methods are using $unset, I am unable to apply the kind of matching required for this. I currently work around this with a manual $pull query, like this:

removeOption: ( value ) ->
    @constructor._collection.update { _id: @_id }, { '$pull': { 'i18n.nl.options': { value: value } } }

While this works, I have to hardcode the i18n field name. It would be preferable to do this through the package API. I'll think about possible ways to implement this in the API.

In addition, I would like to be able to simply invalidate all translations of this property, which I seem to not be able to do right now (every function requires a language argument). But I'll open a seperate issue for this.

rijk commented 10 years ago

An possible way to support this via the API:

@constructor._collection.removeLanguage @_id, 'options': value: value, '*'

In removeLanguage(), check if the fields parameter is an array (current functionality) or an object. If it is an object, it is treated as a $pull query. Note that I've also included a * language parameter, see #3 for more on this.

So the above call would translate internally to:

@.update.apply @, removeTrailingUndefs([selector, {$pullAll: fields}, options, callback])
theosp commented 10 years ago

Hi @rijk ,

Currently, tap:i18n-db supports only basic operations that should be enough for a wide range of usecases. Since I think that for other users there might be more usecases that are not supported, and because I want to keep the API slim to keep it simple for new users and for maintenance reasons. I tend not to add new methods to this package.

I think that the best thing to do here is to have your suggestion, that I definitely think will be useful for other developers, in an extension package for tap:i18n-db that will be maintained and tested separately. If with time will find that majority of tap:i18n-db needs that method we then either merge it to tap:i18n-db or have an umbrella package that will hold tap:i18n-db and its popular extensions.

I encourage you to wrap your workaround to an extension package, if you'll do that I'll advertise the extension in the main README to make sure people will know about it. Also, it might open the gate for more extensions to be written by other tap:i18n-db users. That will benefit all of us.

Thank you very much for helping to think how to improve tap:i18n-db, it's very appreciated.

Daniel

rijk commented 10 years ago

An alternative idea: do you think it would be possible to tap into the collection’s update method, and watch for $pull (and $unset) requests? If so, we can automatically pull from the translations as well.

I know it sounds a little hacky, but I think the result experience would be it “just works”. Translations sticking around after you’ve deleted a property feels a little messy, and something the package should handle, ideally.