Add Project History Page

[Andrew-Dickinson]

Adds a history page (Resolves #426 ), showing all changes to the database.

Implemented using SQLAlchemy-continuum. Per #426, users can opt-out using a new interface on the Settings page and must opt-in to have their IP addresses recorded.

Users visiting the history page after reducing their privacy settings are prompted to delete the previously recorded data:

This PR also includes a reasonably comprehensive set of tests to assure the privacy protections work as intended and that log entries are created for manual & api operations.

[Glandos]

Incredible work. Thank you!

I've hitted some bugs. Let's start with the first use case:

• Add 2 users
• Add 2 bills
• Change the first created bill amount only

Here is the result:


Note that in the history, I have a line saying that both users were removed from the owers list, but this was not the case. I tried to reproduce this with one bill, but the bug doesn't showed up.

Second use case:

• Add 2 users
• Add one bill (Bill 1)
• Delete this bill
• Add a new bill (Bill 2)

Here is the result:


Note that in the history, the first line (so the last event) say that Bill 1 was added, and it should be Bill 2. The amount is also wrong. I suspect the bug comes from the id, that is currently reused from previously deleted bills.

I also have a remark about momentjs, but I'll do it after @eMerzh review.

[Andrew-Dickinson]

It looks like the ID reuse is really a quirk of SQLite. SQLAlchemy-Continuum is assuming that there is not ID reuse. We can direct SQLite to follow the expected behavior of issuing unique IDs for every row by using the sqlite_autoincrement=True flag on the id columns, although the migration for this might be tricky.

[Andrew-Dickinson]

The other bug is potentially a bug in SQLAlchemy-continuum, which would suck. I've reduced it down to a 70 line gist which illustrates it. I can't see any reason why the setup I've created there would cause the behavior it's causing. It's also got some really weird instability, for example if you change one of the bills to people=[u1] and leave the other with people=[u2], the bug goes away. Similarly, if you remove the commit() call from line 54 it also goes away.

[Andrew-Dickinson]

@Andrew Dickinson commented on 13 avr. 2020 à 23:29 UTC+2:

It looks like the ID reuse is really a quirk of SQLite. SQLAlchemy-Continuum is assuming that there is not ID reuse. We can direct SQLite to follow the expected behavior of issuing unique IDs for every row by using the sqlite_autoincrement=True flag on the id columns, although the migration for this might be tricky.

I don't know the details behind activating autoincrement for existing project, but from a user point of view, it should be OK. I'll try to make some tests.

I believe I sorted this out in migration cb038f79982e_sqlite_autoincrement.py. This works when I test it locally, let me know if you see any problems.

[Andrew-Dickinson]

Small enhancement proposal: I saw that in some cases, there is a link to clear project history, or recorded IP addresses. Do you think these links could always be displayed, like the "New bill" button in the bill list?

I wanted to try to keep the UI pretty minimal, but if it's something you guys want it's pretty easy to throw something in there.

[Andrew-Dickinson]

Small enhancement proposal: I saw that in some cases, there is a link to clear project history, or recorded IP addresses. Do you think these links could always be displayed, like the "New bill" button in the bill list?

I wanted to try to keep the UI pretty minimal, but if it's something you guys want it's pretty easy to throw something in there.

Done in 80bc2ac

[Andrew-Dickinson]

I believe the only outstanding issue would be the "test project" concern raised by @indatwood :

I've tested it locally, and here is a quick feedback : when using it on the "test" project, in the options it says that the history is not enabled but an history page exists and records everything.

I'm not clear on what the "test project" is, can someone help me understand?

[almet]

I believe he meant the « demo » project? btw, awesome feature you're implementing, and huge amount of work, thanks!

[Andrew-Dickinson]

As far as I can tell, the demo project has History Enabled by default just like any other project. Maybe this got fixed when I re-did the check-boxes, but it seems good to me now.

@Glandos Anything else you need from me to get this merged?

[Glandos]

No, it's merged now, and I've subscribed to the bug report you opened in continuum to remove the workaround when needed.

Thanks for your awesome work to make this project better!

[zorun]

Starting from this change, when I use alembic to create a new automatic migration (to test something for #557 ), it picks up something strange even if I don't touch the models:

$ python -m ihatemoney.manage db migrate -d ihatemoney/migrations -m test-after-history
INFO  [alembic.autogenerate.compare] Detected removed table 'sqlite_sequence'
  Generating /home/dev/tmp/ihatemoney/ihatemoney/migrations/versions/9c340fd31fff_test_after_history.py ...  done

I have checked that if I do the same thing before this change, no migration is created:

$ git checkout 91ef80ebb712b06b6c48336beeb7f60219b0f062
$ python -m ihatemoney.manage db migrate -d ihatemoney/migrations -m test-before-history
INFO  [alembic.env] No changes in schema detected.

Here is the content of the strange migration:

# revision identifiers, used by Alembic.
revision = '9c340fd31fff'
down_revision = 'cb038f79982e'

from alembic import op
import sqlalchemy as sa

def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_table('sqlite_sequence')
    # ### end Alembic commands ###

def downgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('sqlite_sequence',
    sa.Column('name', sa.NullType(), nullable=True),
    sa.Column('seq', sa.NullType(), nullable=True)
    )
    # ### end Alembic commands ###

[Glandos]

Maybe because of e7848e3?
I am not enough skilled with Alembic to have a clear opinion though.

[zorun]

Right, that seems related.

Now I remember that Alembic was giving me a warning about not taking into account ALTER while applying the migration:

INFO  [alembic.runtime.migration] Running upgrade a67119aa3ee5 -> 6c6fb2b7f229, empty message
INFO  [alembic.runtime.migration] Running upgrade 6c6fb2b7f229 -> 2dcb0c0048dc, autologger
/dev/venv3-ihatemoney/lib/python3.8/site-packages/alembic/ddl/sqlite.py:40: UserWarning: Skipping unsupported ALTER for creation of implicit constraint
Please refer to the batch mode feature which allows for SQLite migrations using a copy-and-move strategy.
INFO  [alembic.runtime.migration] Running upgrade 2dcb0c0048dc -> cb038f79982e, sqlite_autoincrement

@Andrew-Dickinson any idea? I'm using Arch with python 3.8 and sqlite 3.31.1-1

[zorun]

It looks like #577 is also related to this migration.

[Glandos]

@zorun did you still have the issue after @Natim manually update the migration in #579?

[zorun]

@Glandos both issues are still there: warning while applying the sqlite_autoincrement, and spurious detected change when creating a new migration.

I think @Natim 's change only disables the migration for MySQL and PostgreSQL, it does not change the behaviour for sqlite.

[Andrew-Dickinson]

So the migration in question is one that I had to write manually. Albemic wasn't picking up on the fact that I had added sqlite_autoincrement=True to each table per:
https://docs.sqlalchemy.org/en/13/dialects/sqlite.html#sqlite-auto-incrementing-behavior

[Andrew-Dickinson]

Starting from this change, when I use alembic to create a new automatic migration (to test something for #557 ), it picks up something strange even if I don't touch the models:

According to:
https://sqlite.org/fileformat2.html#seqtab

This is a table that SQLite uses to store the highest known primary key for each table. It is automatically added by SQLite when a table with autoincrement set is created. Notably, it can't be deleted:

Once created, the sqlite_sequence table exists in the sqlite_master table forever; it cannot be dropped.

This makes it strange that Albemic would ever add any migrations with respect to it. This table should always be handled by SQLite itself.

[Glandos]

@Andrew-Dickinson The patch you provided in SQLAlchemy-Continuum has been merged, and released in 1.3.10. Do you have the time to check if history work without the workaround, so that we can remove it from our codebase?