-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Stop filtering out all categories that have a year in it #4902
base: main
Are you sure you want to change the base?
Conversation
As a fix for commons-app#750, a validation was added to filter out categories that were of form "... taken on ... 1990" or "... needing ... 2005". The conditional added for the fix was a bit loose than it should be as a result of which it filtered out all categories that had a year in its name. This is incorrect. Fix this by tigheting the conditional so that only categories that have a year, is not the present or previous year AND (contains "taken on" OR "needing") is filtered out. While doing so, turn an implicit precedence into an explicit one.
We are in the 2020s now. So, adjust fix for issue 1029 to include 2020s too.
Codecov Report
@@ Coverage Diff @@
## master #4902 +/- ##
============================================
- Coverage 50.59% 50.58% -0.01%
- Complexity 2228 2229 +1
============================================
Files 338 338
Lines 15599 15599
Branches 1370 1370
============================================
- Hits 7892 7891 -1
+ Misses 7114 7112 -2
- Partials 593 596 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this feature would greatly benefit from a unit test :-)
Photographs taken on 2015-11-08
-> HideAmavenita (ship, 2014)
-> Do not hide
Good point. I'll look into adding the tests. I must mention that I'm not so used to adding unit tests for Android. So, it might take me some time. I'll update here once I'm done with adding the required tests. |
Just wanted to update here regarding the test. I was trying to cook up a test for the mentioned case but somehow the test keeps failing. I think I'll need to look more deeply to write the test properly. Meanwhile, if anyone has any good pointers on what I'm getting wrong, do let me know. |
Hey I think the checkIrrelevantCategoryIsFilteredOut should be inside categoriespresentertest class.
|
I might be totally wrong, but I think we can test the functionality using simpler unit tests like these
|
@sivaraam as mentioned in #5749 (comment) as other prerequisites have been merged now; perhaps this PR can now be rebased to latest master and updated to allow years (except blacklisted ones)? |
Description (required)
Fixes #4901
What changes did you make and why?
Made the conditional tighter so that it only checks for the cases needed by #750 and not more than that.
Tests performed (required)
Tested ProdDebug on Nexus 4 emulator with API level 30.