Remove users longer than 3 months for longer lists #47
Conversation
There was a problem hiding this comment.
Given that you guys are - AFAIK - the only one using this, I'm fine merging as is.
However, I would prefer to see the newly changed behaviour reflected in the UI as well, such that a user can understand why certain users are and aren't visible.
It would have been even better if that's something you can toggle as a filter in the fronted. But given the tech stack - or lack thereof in the front end - that's definitely not trivial, so that's probably never going to happen.
| @@ -39,20 +39,35 @@ public function beforeView() | |||
| usort($byFirstName, function($a, $b) { | |||
| return strcmp(strtolower($a[User::FIELD_FIRST_NAME]), strtolower($b[User::FIELD_FIRST_NAME])); | |||
| }); | |||
| $byFirstName = array_filter($byFirstName, function($a) { | |||
| $timeStamp = strtotime($a['time']); | |||
| $cutOffDate = strtotime('-6 months'); | |||
| $byLastName = array_filter($byLastName, function($a) { | ||
| $timeStamp = strtotime($a['time']); | ||
| $cutOffDate = strtotime('-6 months'); | ||
|
|
||
| return ($timeStamp > $cutOffDate); | ||
| }); |
There was a problem hiding this comment.
Code quality wise I would have factored out the filter function into a separate named function. Then both filters can use the same logic twice.
DRY for only two instances of the code is often just an edge care. So I'm fine keeping it like this. More of a general remark.
|
In case this gets merged. @all-contributors |
|
I've put up a pull request to add @omit01! 🎉 I've put up a pull request to add @Synthetica9! 🎉 |
|
I've pulled the master branch with the prod application. Now everyone gets a crown 🎉. But I don't think this is the intention of the code as described in the PR. The leaderboard page is missing the crowns :(. So it would be cool is it works on that page as well. That page is central in de gamification of the app. Also I get an error when loading the transaction page. The rest of the app works fine. PS: I think you can close #16 |
|
@koen860 I will look into fixing the bug. With the previous update, where I added the recents tab, it was also the case that production didn't work as expected, although I used a copy of the production database. |
Togheter with @Synthetica9, I used the Nerd Night to give LISA some extra features. One of those features is removing the people who are inactive from the larger lists sorted by first and last names.
We filtered this arrays to only use users who had a transaction maximal 3 months ago.