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

Add unread filter to entries pages #2052

Merged
merged 2 commits into from May 27, 2016

Conversation

danbartram
Copy link
Contributor

New feature: Filter Unread

This PR allows users to filter their entries and requiring items to be "unread".

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes*
Documentation no
Translation yes
Fixed tickets #2031
License MIT

* I'm getting a couple of PHPUnit test errors/failures, but they also exist on versions without this change.


Possible Issues

There is some coupling caused by the SQL query that might cause future issues if the schema/Doctrine design changes.

Currently in EntryFilterType.php the column for finding the read/unread status is also the archived status column. This uses the same logic as the "Unread" page view.

However, in the query you can see that the field e.isArchived is hard-coded and won't change if/when the Doctrine models' parameters do.

If there is a better way to design this please let me know, this solution works but it doesn't seem the best design to me.

Add the ability to filter for unread pages in the filters menu.
@nicosomb
Copy link
Member

nicosomb commented May 9, 2016

Hello @danbartram!
Thanks for your PR.

Can you add a new test like this one please? https://github.com/wallabag/wallabag/blob/master/src/Wallabag/CoreBundle/Tests/Controller/EntryControllerTest.php#L455

What happened if you tick the new checkbox and the Archived one?

@danbartram
Copy link
Contributor Author

danbartram commented May 9, 2016

@nicosomb I sure can, sorry for missing the test out. How would you like it resubmitted? As a new PR or as an extra commit?

What happened if you tick the new checkbox and the Archived one?

No entries are returned, as the query will be asking for entries WHERE e.isArchived = true AND WHERE e.isArchived = false.

@nicosomb
Copy link
Member

nicosomb commented May 9, 2016

An extra commit ;-)

Add a new test to the EntryControllerTest collection which checks that
only entries which have not been archived (and are treated as "unread")
are retrieved.
@nicosomb nicosomb added this to the 2.0.5 milestone May 27, 2016
@j0k3r j0k3r merged commit 8394757 into wallabag:master May 27, 2016
@j0k3r
Copy link
Member

j0k3r commented May 27, 2016

👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants