fix: Skip button reloading from cached images #360
Conversation
mahmoud
left a comment
mahmoud
left a comment
There was a problem hiding this comment.
Interesting! I believe skip worked before the new frontend. I half expected this to be primarily frontend code changes, but I see it's only backend changes. Are there supposed to be frontend changes too?
Also, when updating the backend we should probably be adding tests for repro/regression protection, as well.
montage/juror_endpoints.py
Outdated
| result = juror_dao.skip_voting(vote_id, round_id) | ||
|
|
||
| if isinstance(result, InvalidAction): | ||
| return {'error': str(result)}, 400 |
There was a problem hiding this comment.
Doesn't raising InvalidAction do a 400 already?
There was a problem hiding this comment.
Yes, I was just using that to test something else, while I was working on this. I'll remove it soon.
montage/rdb.py
Outdated
| flag_modified(round_juror, 'flags') | ||
|
|
||
| self.rdb_session.add(round_juror) | ||
| self.rdb_session.commit() |
There was a problem hiding this comment.
I believe commit should be handled by the middleware
There was a problem hiding this comment.
Sry about that, I'll remove that line
YES, the frontend changes are absolutely required. For some reason that I checked now, the changes I made on |
|
I've also attached the recording of this fix on the issue. Here |
mahmoud
left a comment
mahmoud
left a comment
There was a problem hiding this comment.
That's more like it! Frontend + backend starting to make sense. Only further idea I have is to enhance the current skip test (which doesn't do any validation really) here:
montage/montage/tests/test_web_basic.py
Lines 440 to 444 in ec3a1a7
We just need to add a confirmation that the backend marked it as skipped, either through the return or another API call.
I've added that test there now, can you check and tell me whether any more changes is required! |
mahmoud
left a comment
mahmoud
left a comment
There was a problem hiding this comment.
looks awesome, thanks so much! will work on getting this deployed asap
2 participants
Description
Fixes #319
This PR fixes the problem with the skip button which does not loads from the backend but instead endlessly loop over the same 10 images which was cached until we accept/reject any image in that set.
The root cause was just a mismatch on the backend, since it was supposed to call
round_juror.flags['skip']but it was trying to read fromround_juror.skipcolumn which does not exist.Moreover the frontend didn't even had a POST method to convey the backend about the skip, due to which the backend never knew about the skip.