Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continue reviewing after accept #767

Merged
merged 10 commits into from Jan 28, 2019
Merged

Continue reviewing after accept #767

merged 10 commits into from Jan 28, 2019

Conversation

rstorey
Copy link
Member

@rstorey rstorey commented Jan 17, 2019

Refs #749

Copy link
Member

@acdha acdha left a comment

Choose a reason for hiding this comment

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

redirect_to_next_reviewable_asset and redirect_to_next_transcribable_asset are very similar. It seems like we could have those functions be almost entirely reusable except for the initial asset queryset filter.

)
potential_assets = potential_assets.filter(
transcription_status=TranscriptionStatus.SUBMITTED,
assettranscriptionreservation=None,
Copy link
Member

Choose a reason for hiding this comment

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

The assettranscriptionreservation shouldn't be relevant for review since that's only used when editing.

@@ -1164,7 +1241,7 @@ def redirect_to_next_transcribable_asset(request, *, campaign_slug):
next_asset=Case(
When(pk__gt=asset_id, then=1), default=0, output_field=IntegerField()
),
).order_by("-next_asset", "-unstarted", "-same_project", "-same_item", "sequence")
).order_by("-next_asset", "-unstarted", "-same_item", "-same_project", "sequence")
Copy link
Member

Choose a reason for hiding this comment

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

This is causing the test to fail because it no longer prioritizes staying in the same project ahead of staying in the same campaign.

acdha
acdha previously approved these changes Jan 22, 2019
@rstorey rstorey changed the title WIP: continue reviewing after accept Continue reviewing after accept Jan 22, 2019
@rstorey
Copy link
Member Author

rstorey commented Jan 22, 2019

My effort to de-dupe the code was fruitless. It caused the never_cache decorator to fail for a reason that I'm not quite sure of. In any case, although the two methods are very similar, they aren't exactly the same and there seems to be some kind of reason why never_cache wants the redirect call to be in the main view method. It was worth a try.

@coveralls
Copy link

coveralls commented Jan 22, 2019

Coverage Status

Coverage decreased (-0.4%) to 69.158% when pulling 0a9a900 on review-after-accept into d8a0989 on master.

@acdha
Copy link
Member

acdha commented Jan 22, 2019

never_cache shouldn't be preventing that – it just adds a couple of headers on the returned Response object so it should be fine as long as we consistently do so.

@rstorey
Copy link
Member Author

rstorey commented Jan 22, 2019

But the returned Response object isn't available in the secondary method for some reason. It seems like it should be, but it isn't. I spent almost an hour trying to figure out why it wasn't there, fooling around with different method signatures to try to get it to work. The effort didn't seem worth it.

concordia/views.py Outdated Show resolved Hide resolved
concordia/views.py Outdated Show resolved Hide resolved
@rstorey rstorey self-assigned this Jan 25, 2019
@rstorey rstorey merged commit 549b916 into master Jan 28, 2019
@rstorey rstorey deleted the review-after-accept branch January 28, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants