Merge Tags

[inukshuk]

• Add a merge function to the tag model. The function should take a db transaction (i.e., the function can throw on error, no need to worry about rollbacks) and two or more tag ids to merge. The merged tag retains name and color from the first tag given and all items tagged with any of the tags in the list. All tags except for the first one should be removed.
• Add tests for the merge function. (Mocked unit tests or tests on a real db connection? If real db, I guess it's enough to use an in memory db with a full, or partial?, project schema).
• Add a Merge command to commands/tag.
• Add a merge action to actions/tag that triggers the Merge command.
• Add keyboard shortcut for merge and for rename tag(s) to TagList
• Implement the Merge command. This will call to the merge model function to actually merge the tags. Then the state has to be updated via a reducer. For this we need to determine what has to be updated (the merged tag, other tags removed, all items involved) and the command needs to dispatch an action with all this information (we can either re-use existing actions that do this, create new ones, or use the merge action itself that is dispatched with the result of the command).
• Add the reducer to actually update the state.
• Add a reverse command for undo (unmerge, explode, ...?). The command should restore all given tags, colors, and all items previously tagged. From the first tag, the originally tagged items should be restored.
• Make TagList context menu work with mutliple selected tags
• Add a tags section (multiple) to the context menu (color, merge, delete)
• Ensure changing color and delete works for multiple tags (may have to adjust actions, reducers, commands)
• Show multi-tags context menu when multiple tags are selected (and keep the selection!)

Closes #144 #671

[inukshuk]

Using this here and below is potentially dangerous. It will commonly happen that these helpers are invoked directly and not via the common object used as the default export. We may also change the way we export these model functions.

[caro401]

is there another way to access that function without using this? I don't understand the JS export system well enough to properly understand this issue

[inukshuk]

The issue itself is not so much about the module/export system (but it's likely to be exposed because of it).

The problem is that this is only bound when the function is invoked (unless it's a bound or an arrow function!). If we reference this.items here, this may or may not work depending on how the function is called. The way use these functions makes it very likely that we'll have to pass them around to be invoked later, which would likely break. For example:

let a = { f() { return this.g() }, g() { return '!' } }
a.f() //-> '!'
setTimeout(a.f, 25) //-> This will fail. It's similar to:
let f = a.f
f() //-> Which also fails.

It's very common that we have to pass these type of functions around (e.g., when we use them in Commands). The module/export dynamics also play a role, because named exports are the norm with ESM we may want to switch to named exports going forward so that we can import { merge } from './models/tags.js'.

In summary, you can easily address this by hoisting the items function similar to the load function (which is referenced by create). If we want to export all functions individually that's probably what we should do anyway, for all the model functions, but then we probably need address a few naming issues (e.g., the 'delete' function would clash with the keyword; 'items' in isolation is probably too cryptic etc..). Another alternative would be to still keep everything in a namespace object like we currently do, but also store it in a local variable for referencing purposes.

[inukshuk]

In reference to the earlier example, of course setTimeout(a.f.bind(a), 25) would work, but it's not good if we always need to remember when to bind functions and when it's safe to pass them around.

[inukshuk]

Lets fold the loop into the DB command. This way we should be able to send a single insert command for all items to the DB instead of one command for each item.

[inukshuk]

Yes, there should be! We can probably use REPLACE INTO which is just a SQLite shorthand for INSERT OR REPLACE INTO. But maybe it would be better to use ON CONFLICT IGNORE because REPLACE will remove the old tagging first and we probably want to keep the original? (Our 'query generator' classes are far from complete; if we use this here we can add a way to set e.g. onConflict('ignore') on the insert statement)

[caro401]

would INSERT OR IGNORE INTO be appropriate here? I didn't know SQLite had all these extra options around insert

[inukshuk]

Yes, I think INSERT OR IGNORE fits perfectly here. It applies to primary key constraints so we can use that instead of handling the error, or checking each tag/item combination first.

[inukshuk]

We must be careful not to swallow real errors here (so make sure to throw). Because this function should be called with a transaction, we can be sure everything will be rolled back if we the function throws an error.

[inukshuk]

Here is a good illustration about the potential dangers of using this in the model functions. If we use this in create here it will fail.

[inukshuk]

I think this is fine, but two things come to mind: I'd be interested to explore using an in-memory db for such kind of tests, because they're almost certainly going to be faster and we won't have to clean up the file afterwards. If I'm not mistaken then you should be able to call Database.create with path ":memory:" to create an in-memory db. We could adjust the mkdbtmp helper to skip any file related actions if that's the path.

The second thought was that we could integrate this into the fixture helpers maybe.

let p = F.project.create()

And this works sort of like mkdbtmp in that it adds before and after hooks which create the and close the db connection accessible under p.current (sort of like React's refs). In fact we should change mkdbtmp to also use that pattern then we can skip the callback and the tests are much easier to read. What do you say?

[inukshuk]

I've explored re-writing the mkdbtmp helper to return a React-like ref object, with the before/after hooks managing its current value. I think this way it's more straightforward to use, although having to type .current everywhere is somewhat annoying. What do you think about this?

I've also changed some of the tests in the process where I thought it would be useful. Most importantly probably, when you test for exceptions or rejections always use the .throws() or .rejected / .rejectedWith() matchers: that should be preferred over adding try/catch blocks to the tests themselves.

I may have made some other instructive changes -- just let me know if you anything catches your eye.

The most confounding thing though, is that I broke your merge tests and I haven't figured out why yet. The failure is so bizarre that I wonder if it's due to some underlying bug so I committed this with the failure for now hoping that it will be instructive for us to figure out what's going on.

Here is what I know so far:

The before hook that inserts tags and items fails. The error is caused by the trigger that adds metadata values to the full-text index. Specifically, it looks like the fts_metadata virtual table does not exist. At first I assumed this may have to do with the tests using an in-memory db, but the error is the same when I switch to using a temporary file. Next, I figured that there was an issue with the db-pool abstraction, that somehow we were opening a connection to an empty db. But I think I can rule that out too, when sending consecutive queries over the same connection in that before handler I see that fts_metadata exists in the sqlite_schema table, but querying e.g. select * from fts_metadata limit 1 still fails -- which is quite baffling.

[inukshuk]

OK I don't know yet why this fails, but I have isolated the issue now: it happens when you load the project schema and then keep using the db connection without closing it. Previously mkdbtmp used Database.create which created the db and closed it right away. This wasn't really on purpose originally, we just re-used the Database.create script for the test helper the way it was. So it looks like this was an unintentional workaround for an issue with our db schema we didn't know existed.

[inukshuk]

I think this is because virtual tables are registered per connection. When we restore the schema, the virtual table is added to the sqlite_schema, but probably not registered with the connection. We need to find out if there is a better way to dump and restore virtual tables; if there isn't that really rules out in-memory dbs if virtual tables are used. (A workaround would be to run all migrations instead to re-create the schema)

[inukshuk]

Seems like there's a simple solution!

[inukshuk]

OK I rebased the branch again and added some minor changes to make the tests more concise and this should all be working again now. The jury is still out on the new mkdbtmp helper: that we can use in-memory DBs now is definitely a plus which should definitely stay. I'm not fully convinced of using the React-style ref syntax. I do feel that it's much easier to understand because it works without the callback; on the other hand .current is a bit verbose. We could pick a shorthand of course, I don't know.

It also looks like these DB tests fail consistently on the Windows CI again. Interestingly also the tests using the in-memory DB are failing so contrary to my priors, this might not be a file system issue after all!