Issue 282: Fixed challenge listing sorting by next submission or checkpoint date #357
Conversation
birdofpreyru
left a comment
birdofpreyru
left a comment
There was a problem hiding this comment.
@Colinh84 I think we need some corrections here, or might be I misunderstood something
| name: 'My Challenges', | ||
| sorts: [ | ||
| SORTS.MOST_RECENT, | ||
| SORTS.TIME_TO_SUBMIT, |
There was a problem hiding this comment.
Not sure, why do we need this change (see also further comments).
There was a problem hiding this comment.
@birdofpreyru The "open for registration" listing was already using SORTS.PHASE_END_TIME, which is the sort that I modified to fix the sort bug; since "my challenges" should be sorted the same way, I just updated it to use the same sort. The SORTS.TIME_TO_SUBMIT sort only sorts by final submission end time, while SORTS.PHASE_END_TIME sorts by end of next submission phase, including next checkpoints.
| function filterByRegistrationOpen(challenge, state) { | ||
| if (_.isUndefined(state.registrationOpen)) return true; | ||
| const isRegOpen = () => { | ||
| if ('registrationOpen' in challenge) { |
There was a problem hiding this comment.
No, please do such checks just as if (challenge.registrationOpen) {.
I am also not sure, why do we need to touch this filter at all?
There was a problem hiding this comment.
@birdofpreyru This filter needed to be changed because the "open registration" challenge listing included results for stalled challenges which were closed a long time ago, but still appeared because they had a "status' key set to ACTIVE. There is another key in all challenge results that was not being checked in the filter called "registrationOpen", and that key was properly set to "No" on these stalled challenges, so doing this extra check here removes these old stalled, closed challenges from the open registration listings.
| }, | ||
| [SORTS.PHASE_END_TIME]: { | ||
| func: (a, b) => a.currentPhaseRemainingTime - b.currentPhaseRemainingTime, | ||
| func: (a, b) => { |
There was a problem hiding this comment.
I don't competely follow, why do you modify this sort function? I believe, the original code was correct, wasn't it? It trully compared the time till the phase end (which could be any phase, not only checkpoint and submission). The only thing that wasn't good was the measleading name of this sort.
On the other hand, the code itself seems fine logically, just had to be put into [SORTS.TIME_TO_SUBMIT]
There was a problem hiding this comment.
@birdofpreyru This was the main source of the bug. The original sort here was actually not sorting anything and returned the results unchanged from the server, because the "currentPhaseRemainingTime" is a key from the V2 API which is no longer in use for grabbing challenge listings and is not present in the V3 API. None of the challenge results contain this key anymore. This SORT.PHASE_END_TIME was the sort that was being used for "time to submit" in open for registration listings, was labeled as such and was used nowhere else, which is why I modified this one.
| func: (a, b) => a.currentPhaseRemainingTime - b.currentPhaseRemainingTime, | ||
| func: (a, b) => { | ||
| function nextSubEndDate(o) { | ||
| if (("checkpointSubmissionEndDate" in o) && moment(o.checkpointSubmissionEndDate).isAfter()) { |
There was a problem hiding this comment.
The first part of condition should be just o.checkpointSubmissionEndDate && ...
|
@birdofpreyru I updated the branch with requested changes. Also see my previous replies to code comments. |
2 participants
#282