Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added tagging functionality and filtering by date, amount, payer, tag #557

Closed
wants to merge 7 commits into from

Conversation

allisongu
Copy link

@allisongu allisongu commented Apr 20, 2020

@nachow16 and I wrote this code to fix #430 and fix #55

@Glandos
Copy link
Member

Glandos commented Apr 21, 2020

Hi!

Nice improvement. It seems simpler that what I expected, so good job!

There is however some issues for now, please have a look.

@allisongu
Copy link
Author

So I've run make create-database-revision and it created the upgrade and downgrade functions that are commented out in 321d3f042213_added_tag_column_to_bill_model. When I run the tests, I fail every single one due to "table sqlite_sequence may not be dropped" and "table bill_version already exists". In the non-commented out code is my edit of the migration file that fails 6 tests due to "duplicate column name: tag". Really not sure how to fix this, does anyone know what I should do?

@zorun
Copy link
Collaborator

zorun commented Apr 23, 2020

This sounds good! Being able to search is especially important now that we have paginated output (#480), because a simple browser-based text search won't show everything.

Is it possible to have a screenshot of how the search functionality would look like?

@allisongu
Copy link
Author

allisongu commented Apr 23, 2020

This sounds good! Being able to search is especially important now that we have paginated output (#480), because a simple browser-based text search won't show everything.

Is it possible to have a screenshot of how the search functionality would look like?

We used a previous PR's code to start us off and this is how it ended up looking:
Screen Shot 2020-04-23 at 12 26 05 PM

However, I do not think the original code we based ours off of will work with pagination. (PR #511)

@zorun
Copy link
Collaborator

zorun commented Apr 23, 2020

@allisongu great, thanks!

If you want to test what happens with pagination, you can change the number of items per page to a really low value in ihatemoney/web.py:

.paginate(per_page=100, error_out=True)

Although I see now that we also have javascript-based pagination and sorting (#538), I'll have to check how the two pagination methods interact...

Edit: forget that last comment, #538 is only about the admin dashboard, not the list of bills!

@zorun
Copy link
Collaborator

zorun commented Apr 23, 2020

So I've run make create-database-revision and it created the upgrade and downgrade functions that are commented out in 321d3f042213_added_tag_column_to_bill_model. When I run the tests, I fail every single one due to "table sqlite_sequence may not be dropped" and "table bill_version already exists". In the non-commented out code is my edit of the migration file that fails 6 tests due to "duplicate column name: tag". Really not sure how to fix this, does anyone know what I should do?

To generate migrations, you need to make sure that you have an up-to-date database.
The simplest way is to start a new database from scratch:

  • remove or rename any leftover database from previous testing (it should be at /tmp/ihatemoney.db)
  • start from the latest ihatemoney code
  • run make serve or python run.py as explained in the contribution guide: before running the dev server, it will create the database and apply all existing migrations
  • stop the dev server
  • apply your code changes
  • create the migration with create-database-revision

It should pick up your changes. I tested it and it also included something strange and completely unrelated about sqlite_sequence:

INFO  [alembic.autogenerate.compare] Detected removed table 'sqlite_sequence'
INFO  [alembic.autogenerate.compare] Detected added column 'bill.tag'
INFO  [alembic.autogenerate.compare] Detected added column 'bill_version.tag'

If something like this happens (it might depend on the system you're using...), you would need to manually edit the migration file to only keep the bits related to the new field.

@zorun zorun mentioned this pull request Apr 23, 2020
@allisongu
Copy link
Author

allisongu commented Apr 23, 2020

@zorun The instructions to get the database revision fixed worked! I just had to delete the lines in the migration file that had to do with sqlite_sequence. Thanks so much :) That might be a good thing to include in the Contributing Guidelines for other newbies too.

I tested out our search functionality with a low number of bills per page (1 haha) and it does not search through all bills currently, only the ones shown on the page.

@zorun
Copy link
Collaborator

zorun commented Apr 23, 2020

Great! The documentation update is a good idea: #569

For the issue with pagination, it needs some thinking. One way would be to do the searching/filtering on the server side. However, it means that:

  • either we reload the entire page when the search/filter conditions have changed. This doesn't seem very user-friendly.
  • either we need to load the list of bills asynchronously. This is probably a good idea in itself, but quite a large change.

Any other ideas?

@zorun
Copy link
Collaborator

zorun commented Apr 25, 2020

@allisongu what do you think of doing this in two steps?

The first step could be to just add tags, and provide simple server-side filtering on tags. Regarding the UI, maybe each tag in the bill description could be a link that filters down on this tag? Or also add a filtering bar like you did, but only for tags? Filtering would need to reload the page, which is fine as a first step I think.

The second step would then add more powerful searching (by date, payer, etc), and look for a better solution to the pagination problem & asynchronous loading.

This is just a suggestion and more ideas are welcome.

@eMerzh
Copy link
Contributor

eMerzh commented Apr 26, 2020

i'll +1 @zorun , maybe first doing small steps.

Also, please keep in mind that we want to try to keep this page as light as possible, so adding a full blown search form in a big card might be a bit too much.

@Glandos
Copy link
Member

Glandos commented Apr 26, 2020

We are on the way to discover that filtering is complex, especially from the UI point of view.
But the tag part can be simple in both implementation and UI.

@allisongu, do you think you can open a new request with only the tag management in the database? As @zorun mentioned, you can also add a browse-by-tag feature, like this one: https://www.koreus.com/tag/chaton When you click on a new tag, it filters on both selected tags.
In my opinion, the tag list could be shown on demand, with a "Filter" button. In this list, we can later add a more complex filtering header.

@indatwood
Copy link
Contributor

Hi, here we are with this? It seems kinda stalled (last discussion and commit 6 months ago) but the feature is cool :-)

@allisongu, do you want to finish your work on this or should we let it go?

@allisongu
Copy link
Author

@indatwood Hey, unfortunately I don't have the time to contribute to this feature anymore so unless someone else wants to pick it up, i guess we should let it go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to search / filter expenses Add tags feature
5 participants