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

(WIP) Story 145: Serve ad from heroku, not psiturk #425

Closed

Conversation

davisagli
Copy link
Contributor

(still in progress; opening PR for ease of review)

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

@DallingerBot
Copy link
Contributor

DallingerBot commented Jan 23, 2017

2 Warnings
⚠️ Please use more than one word.
25823ea
⚠️ Please limit commit subject line to 50 characters.
41aa3ed

Generated by 🚫 danger

# them to start the task again.
raise ExperimentError('already_started_exp_mturk')
elif status == COMPLETED:
if status == 'working' and part.end_time is not None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be if status == 'submitted': and we should make sure that the /worker_complete route updates the status to 'submitted'

Good catch on the difference in how the Dallinger participant model handles the status field.

Copy link
Member

Choose a reason for hiding this comment

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

That's not the case. I tried this at first, before I completely understood how the notifications were working. 'submitted' means that externalSubmit has been called and the HIT is complete. A failure callback could still be received from mturk after worker_complete.

worker_complete is called in the popup window, the job of which is essentially to mark the user's session as being over. It then refreshes its window.opener and closes itself. The window.opener is the iframe inside mechanical turk, which then needs to show the externalSubmit button, which actually ends the task. As soon as externalSubmit is called, the parent of that iframe, the HIT info page on MTurk changes to being a confirmed page and a callback is made to set the status to be 'submitted'. If the iframe is reloaded before the completed state it shows an error.

One other way of doing this would be to change the state enum to have a state between 'working' and 'submitted', like 'readytosubmit' and have worker_complete update the status there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, right, thanks for clarifying.

{% if mode == 'debug' %}
<p>If this were a real HIT, you would push a button to finish at this stage.
{% else %}
<p>To complete the HIT, simply press the button below.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sandbox/production modes, is there a reason we can't or don't want to submit the form automatically?

For debug mode, I think we still want to have a form, which submits to an endpoint that calls dallinger.experiment_server.experiment_server._debug_notify the way that the /ad_address endpoint used to. That way we can also debug things that happen in response to notifications when working locally.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't consider changing the existing user-facing behaviour to be a part of this story, but there is an argument for it. However, when the submit button is hit the mturk window changes, so it's possible someone would get lost as to where their session was.

As to the _debug_notify, that's something I missed. I wasn't aware that ad_address was calling it. However, as ad_address is called in dallinger.js, I think it should be called automatically there rather than from a manual button press?

if self.config.get('launch_in_sandbox_mode'):
lookup_url = "https://workersandbox.mturk.com/mturk/preview?groupId={type_id}"
else:
lookup_url = "https://worker.mturk.com/mturk/preview?groupId={type_id}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is failing in tests because type_id is not there.

Copy link
Member

Choose a reason for hiding this comment

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

I did not test this on the production environment, only in the sandbox. What do we have available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant that running tox fails.

Copy link
Member

Choose a reason for hiding this comment

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

So it does. Looks like we need to return info from the mock's create_hit.

@@ -1 +1 @@
dallinger == 2.7.0
-e git://github.com/Dallinger/Dallinger.git@development#egg=dallinger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please don't include this change.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, accidental adding. I never intended for that to be committed.

@codecov-io
Copy link

codecov-io commented Jan 25, 2017

Current coverage is 51.90% (diff: 18.91%)

Merging #425 into stories/137-mturkrecruiter-dev-rebase will increase coverage by 0.60%

@@           stories/137-mturkrecruiter-dev-rebase       #425   diff @@
=======================================================================
  Files                                         25         25          
  Lines                                       2649       2676    +27   
  Methods                                        0          0          
  Messages                                       0          0          
  Branches                                     418        425     +7   
=======================================================================
+ Hits                                        1359       1389    +30   
+ Misses                                      1155       1147     -8   
- Partials                                     135        140     +5   

Powered by Codecov. Last update 10cc683...c2e0d5b

@davisagli davisagli deleted the stories/145-ad-server branch March 29, 2017 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants