Fix for "Mentioned issues are not sorted by date" #2663
Conversation
| @@ -24,6 +24,14 @@ extension NotificationViewModel { | |||
| } | |||
| } | |||
|
|
|||
| // sorting Array<InboxDashboardModel> by date for mentioned filter type | |||
| extension Array where Element == InboxDashboardModel { | |||
| func sortedMentionedByDate(_ predicate: Bool = true, filter: V3IssuesRequest.FilterType) -> [Element] { | |||
There was a problem hiding this comment.
Hm, what's the reason for the predicate? When would you want to call this with false? That would just no-op, if I understand this correctly 😄
There was a problem hiding this comment.
I thought it was a good place for feature toggle. I removed this argument.
| @@ -24,6 +24,14 @@ extension NotificationViewModel { | |||
| } | |||
| } | |||
|
|
|||
| // sorting Array<InboxDashboardModel> by date for mentioned filter type | |||
| extension Array where Element == InboxDashboardModel { | |||
There was a problem hiding this comment.
It would also be 💯 if we can add a test for this one!
There was a problem hiding this comment.
I added tests for this functionality.
|
Thanks for picking this up @akarataev! |
BasThomas
added
😴 awaiting changes
akarataev
force-pushed
the
2649-mentioned-sorted-by-date
branch
from
2a46011
to
c416fa7
Compare
|
Hello @BasThomas, I made a changes, please see it. |
BasThomas
added
💤 awaiting review
There was a problem hiding this comment.
Looks good. I just remembered that we'd probably want to give this as an option though, instead of, like Ryan said, doing manipulation on the client. What do you think? @akarataev?
| XCTAssert(zip(mentionedSorted, | ||
| mentionedSorted.sorted(by: {$0.date > $1.date})) | ||
| .map { $0.date == $1.date } | ||
| .reduce(true) { $0 && $1 }, "Mentioned is not sorted by day") |
There was a problem hiding this comment.
You can use SE-0207's allSatisfy here ;)
There was a problem hiding this comment.
Wow 😲
I did not know about it. Fixed it.
| isPullRequest: false, state: .open | ||
| ) | ||
| } | ||
| static func provide(number: Int, with interval: TimeInterval = TimeInterval(86400)) -> [InboxDashboardModel] { |
There was a problem hiding this comment.
Hm.. I thought it was a linter configuration.
BasThomas
added
😴 awaiting changes
akarataev
force-pushed
the
2649-mentioned-sorted-by-date
branch
from
c416fa7
to
75003f3
Compare
|
I can help design UI for this option. Do you have any suggestions? |
|
What do you think about a filter (a là Mail.app on iOS) in the top right? Would only be there for Take that advice/opinion with a (big) grain of salt though; I'm definitely not a designer 😄 |
|
I can prepare several design options. How do you look at closing this PR under feature flag? And the development of the interface to make a separate task. |
|
Hm, I don't think we've used feature flags for that before. Not opposed to it if we keep it documented in a document as well. |
|
I don’t think giving sorted/unsorted options removes the issue Ryan was worried about since if a user selected the Sorted option we would be sorting on the client and possibly causing some out of sync issue with GitHub. Not sure why sorting would cause an issue but Ryan thought it might. Maybe wait for Ryan before we officially merge ? Sent with GitHawk |
|
@Huddie, |
|
I agree with @Huddie, the last comment here was Ryan’s comment on it. I believe i was first to bring this all up, so trust me I’m all in favor of the sort in itself but sounds like Ryan wanted to keep things as dumbed down as possible so hopefully we can touch base with him on this before merging when he returns. |
|
OK, let's leave this one open for now then 👍 |
Issue mentioned in #2649
I've implement sorting
Array<InboxDashboardModel>by date for mentioned filter type.