Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Don't cache subjects queue #356

Merged
merged 6 commits into from
Jan 15, 2018
Merged

Don't cache subjects queue #356

merged 6 commits into from
Jan 15, 2018

Conversation

simoneduca
Copy link
Contributor

Fix #355

@simoneduca simoneduca requested a review from camallen January 3, 2018 16:01
Copy link
Contributor

@camallen camallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i haven't tested this but the code LGTM

@simoneduca
Copy link
Contributor Author

@camallen mind checking the behaviour here? https://preview.zooniverse.org/shakespearesworld

@camallen
Copy link
Contributor

camallen commented Jan 3, 2018

@simoneduca that link doesn't use local storage for subjects, however it doesn't set the current subject and it persists the annotations across subject reloads thus might incorrectly submit annotations for the wrong subject.

If i start annotating a subject should it remember this and ask me before reloading / reload that subject that i've been working on?

@simoneduca
Copy link
Contributor Author

Ah yes, my bad. I had that check/warning system in place already, which is made useless by my latest changes. I'll store the current subject and clear once the work is submitted or cancelled.

@simoneduca
Copy link
Contributor Author

@camallen and I have figured out what was wrong with the subjects queue. I've just published the latest changes to https://preview.zooniverse.org/shakespearesworld/#!/
@VVH please test if the app behaves as expected now. Please note that on first loading the page you may see the "unsaved work" warning. We're trying to fix that too.

@VVH
Copy link
Contributor

VVH commented Jan 11, 2018

This seems to be working fine for me, but it would be best to ask those who reported the bug on Talk, including mutabilitie, I think?

@camallen
Copy link
Contributor

I've messaged mutabilitie asking her to check.

@simoneduca
Copy link
Contributor Author

Thanks Cam.

@@ -161,7 +153,7 @@ function SubjectsFactory($q, AnnotationsFactory, localStorageService, zooAPI, zo
}

function _setCurrent() {
_data.current = _queue.shift();
_data.current = _queue[Math.floor(Math.random() * _queue.length)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.shift() removes the first element from an array, but the new code here doesn't change the _queue array. Is that intentional? It looks like you will always serve the same small group of subjects to the volunteers with this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I just missed that, thanks! I'll push a fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eatyourgreens please check last commit

@@ -161,7 +153,8 @@ function SubjectsFactory($q, AnnotationsFactory, localStorageService, zooAPI, zo
}

function _setCurrent() {
_data.current = _queue.shift();
var randonIndex = Math.floor(Math.random() * _queue.length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in random.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@simoneduca simoneduca merged commit 75725a2 into master Jan 15, 2018
@simoneduca simoneduca deleted the avoid-stale-subjects-queue branch January 15, 2018 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants