Jimmy,

I spent some time reviewing the module. I didn't look at the code you wrote. I just used the UI and enabled the module and tried to do things with it.

It looks like this is in heavy development since it was pretty buggy. Here are some notes. You will probably already be aware of and have fixed some bugs. Other issues are best-practice, usability and UI issues.

  • Don't use $GLOBALS for uts_installed. What's wrong with a drupal variable or module_exists()?
  • if (isset($_COOKIE['uts_session']) &&
    isset($_COOKIE['uts_session_active']) && $_COOKIE['uts_session_active']) {
  • is bad style and difficult to read. if ($_COOKIE['uts_session'] && $_COOKIE['uts_session_active']) is plenty efficient and sufficient.
  • Why isn't this in hook_init() with a very light weight?
  • Does UTS remember all of the extra database tables it creates for a test session in order to remove them later?
  • Don't disable the module on error (after module number).
  • Give the help text inline in Drupal on respective pages to guide the user from one step to the next in the intial task of creating a study and tasks and inviting participants.
  • The help text, menu items and their order could make most of your help text redundant if designed task-centrally.
  • admin/uts/tasks and admin/uts/analyze are 'trees' in the menu but does not expand and have no children (when empty).
  • Empty lists need help text explaining the problem and offering solutions.
  • I'm not sure what the purpose of admin/uts/tasks is when tasks only really have meaning in the context of a study. I think I'm forgetting something here.
  • The add study/task node form seems to be broken, not title or body field and preg_match() error. Many fields are irrelevant and would be nice to hide; comment settings, authoring information, menu settings etc. I'm not sure implementing tasks and studies as nodes is wise.

Lets discuss these. Let me know if you think any of them are worth breaking out into their own issue nodes on d.o.

Comments

boombatower’s picture

boombatower commented
  • $GLOBALS is used because it ensures that the code has been installed, not the actual module.
  • Removing the isset() works logically, but creates notices which are not preferable (especially from talking with webchick :))
  • The database prefix needs to be changed prior to hook_init() being called. This is the same approached used in the SimpleTest framework in both 6.x and 7.x (core).
  • UTS uses a database prefix for the test environment. It simply sets the database prefix and removes all the tables via hook_shema().
  • Don't disable the module on error (after module number).
    Not sure what you mean by that. If you are refering to the data collection plug-ins they are disabled if they are not required and not supported by the clients computer specs. It is assumed that they will attempt to record if enabled. They are only disabled in the testing environment.
  • Yea, the help text is by far not complete. Just started on it as it is the last thing on the project schedual.
  • Work needs to be done on user flow. Part of the problem is the node form redirects don't appear to work. Webchick was going to help me track it down, but as of now she hasn't gotten around to it.
  • admin/uts/tasks and admin/uts/analyze are 'trees' in the menu but does not expand and have no children (when empty).
    I will look into that.
  • Right, the empty list messages are on my TODO list. :)
  • There was a long discussion about being able to have a "bank" of tasks that were able to be reused and unrelated to studies. That is the purpose of that page. Eventually I will change the import page to clone them instead of just changing their parent study. That will allow the "bank" of tasks to be used over and over.
  • I don't receive the errors when viewing node forms as you described. Please provide the error messages and how to recreate them and I will look into it. As for hiding the extra fields I can look into that. I assume that can be done with hook_form_alter() or something similar?

After some talks with webchick in IRC we agreed that I should release a beta soon since the module is quiet functional and get more feedback. Do you feel there are any major things I should fix before then?

I will clean up my TODO list (so it is readable by other humans) and post it in an issue so we prioritize things and tweak the list. Now that the module is fairly complete the list is reasonable in size. ;)

The main items left (as I see it) are documentation, and more plug-ins (out of scope). Obviously there is still a fair amount of clean-up.

Bevan’s picture

Bevan commented

> The database prefix needs to be changed prior to hook_init() being called. This is the same approached used in the SimpleTest framework in both 6.x and 7.x (core).

Can this be set in uts.module outside of any function? Is this any better than requiring the user put it in settings.php?

  • After enabling the module the message "You must add code to the setting.php file. Please read the INSTALL.txt file in the uts module directory for more details." error message is displayed and the module is NOT enabled. Expected behaviour with modules with irregular dependencies like this is that the module remains enabled, but doesn't do anything until the dependency has been met, and possibly sets the error message on every page request if the dependency is not met.
  • Link to INSTALL.txt in the above message using l() and drupal_get_path().
  • Steps on admin/uts are useful but make it more useful by linking 'create a study' to node/add/uts_study, 'add tasks' to node/add/uts_study.
  • Why would a UTE want to limit the max number of participants in a study? This field is redundant IMO. At least make it optional. At study-creation time a UTS is not likely to know what to put here.
  • What is the 'Task Completion' type? Why are there no other types?
  • Bring the UTE back to admin/uts[/*] after creating UTS nodes.
  • Study and task nodes are promoted to front page by default.
  • Don't use nodes for non-nodish objects (studies, tasks, environments), or at least form_alter-out the irrelevant form fields.
  • Environments needs help text explaining what an environment is.
  • Consider shipping with a default environment.
  • Merge /uts and /uts/participate into one page. No need to separate them out.
  • Add '_' char to the end of $db_prefix so that table names are easily decipherable when browsing the database or reviewing SQL.
  • Having to click 'start' button twice on /uts/participate to start a test session is confusing. Re-think this UI and perhaps split into two separate pages/forms.
  • It's not obvious for the participant that they need to click 'Start' in the header bar before they can commence the test. Either start automatically when entering the test site, or make this UI a bit more intrusive. Perhaps it can be a thickbox when action is required, and shrinks to the top when a user is doing a task.
  • Placement of feedback UI doesn't make it clear that it's not part of the test site, but part of the study in which they are partaking. I think a new window is appropriate for this. Alternatively it could somehow be integrated into the bar that controls the test with the 'start', 'quit' and 'done' buttons.
  • There is no conversation with the feedback / 'live text' UI and the participant. It should feel/behave more like an IM window.
  • Consider renaming 'live text' to 'live feedback'.
  • How is a participant supposed to log in to their test session site? No username and login are not given to the UTE nor the participant.
  • As a participant, the test skipped to task 2 when I tried to log in, even though I didn't click 'Done'. I couldn't log in. (Missing have username and password?)
  • Consider either not destroying the participant's database, or dumping it to file before destroying it. The state of the database after a test can be useful to understand and analyze what a user did in their test session. Currently we are destroying all of this data permanently.
  • Are we capturing data that participants enter in forms during their test session? We should be and I believe this is simple enough to be in scope for GSoC. Display (on it's own page) with drupal_render() or even just var_export() is sufficient for now.
  • The UTS menu items are not available after returning to the host instance of Drupal from a test session. Going to admin/build/modules (but not submitting) restores them.
  • The time that a test session was initiated should be included on the list of sessions in /admin/uts/analyze/NID.
  • 'User path', 'live text' and other non-stream data should be displayed together on a 'timeline' on the analyze page -- not on separate tabs. For now the table is sufficient as a 'timeline' -- but consider integrating timeline module (http://drupal.org/project/timeline) or implementing another more useful UI.
  • There are no empty texts for Analyze pages / tabs.
  • On a page like admin/uts/analyze/1/uts_path/list it's possible to see 'user paths' from multiple participants in one table, which is not useful and confusing.
  • Consider if an 'invite participants' mailer UI and system is in scope.
Bojhan’s picture

Bojhan commented

Next time make it an order list, so I can respond by giving the number I am responding to.

"Why would a UTE want to limit the max number of participants in a study? This field is redundant IMO. At least make it optional. At study-creation time a UTS is not likely to know what to put here."

I don't really get where you are coming from on this one, you don't want to run a study endlessly. Especially as the participant selection at the moment isn't created, we would just set a link somewhere people can use to start a study. There are some common practices for how many people should participate in a usability study (usually 8), so why cant we just put that in a line under that field?

"What is the 'Task Completion' type? Why are there no other types?"

Its the usability method we are applying, in the usability world it would be refer as task analyses. But I asked around and people seemed to understand the term task completion more then task analyses.

The particpant workflow could certainly use some work, ideally its just a start test button overlaying everything.

Reviews on the UI are not really what I think you should review at the moment, since indeed it needs a whole lot of work still.

boombatower’s picture

boombatower commented

> Can this be set in uts.module outside of any function? Is this any better than requiring the user put it in settings.php?

No, for the same reasons SimpleTest can't do that. When Drupal is boostrapped it needs to be changed.

  1. The code uses hook_requirements() to check for the code. I can change the severity, but the UTS won't run properly if the code is not installed so it would seem proper to issue and error. By convention the core won't enable modules with requirements not met. So I can change this to lower severity like warning if you want, but my personal opinion is to leave it.
  2. Will add link to INSTALL.txt.
  3. Will add links.
  4. The original intention was the make the field allow for blank, but it looks as though I have forgotten to do that. Limiting the number of participants can be useful so that you don't get more data then you need (waste participants time). It will auto-close the study. I can get rid of the field, but I'm not sure how it is redundant as there is no other field like it.
  5. At the moment there are no other types. Have in mind others one would be "anonymous watch" meaning it just records certain data from people viewing the site, but doesn't tell them. Would be useful for seeing how people use an interface or site layout, could just record click heatmap or such.
  6. Would like to redirect back to admin/uts/*, but the node API form redirects don't appear to work. You will notice it outputs the URL it would like to goto, but does not work. I have talked with webchick and we will look into it, but she has been rather busy lately. drupal_goto would work, but is not preferred as it can mess up other modules.
  7. I'll look into not promoting them, but I'm not sure I can set that.
  8. Large discussion about using nodes or not and webchick and I felt it was better idea and would allow for later expansion (at this point changing would be essential starting over) I'll look into altering out the other fieldsets, although low priority.
  9. Sure help text needs to be updated all over, last item on my schedule.
  10. Default environments would be good. (Lots of database space, so we may want to think about it)
  11. I'll merge /uts and /uts/participate.
  12. I can add the _ char, just followed SimpleTest conversion.
  13. What click twice allows is for environment to be created them session resumed. That will allow people to come back later and resume, getting to the page the same way they did before. It could be done differently, but I think we need to look into that as it displays messages which are fairly clear, maybe need better messages.
  14. Sure make a large border or backcolor change would be good.
  15. I would like to make feedback hide and expand using JavaScript, just haven't gotten to it yet. That work?
  16. Who will be responding to feedback? Wasn't planning on making a IM client. The javascript and interface for the feedback item is on my list, I've been shotting for functionality.
  17. Live feedback sounds better. Will change. (will loose CVS history :( )
  18. Just getting to the whole login issue. Part of that is solved by using a base environment which could be setup with any number of users. Right now it generates a user 'admin' with password 'utsadmin', but that is not yet worked into the interface. (still dev code)
  19. Did you run out of time? At this point it just switches you without much notification of what happened.
  20. I can leave the database if you want. Perhabs an option per-study or just global UTS config option?
  21. I can work on capturing data, haven't looked into much, but webchick and I were thinking it would require a fair amount of work.
  22. The menu items disappearing is a bug introduced with the new base environment stuff (post alpha 2) which I am aware of. (still dev code)
  23. Only question would be that the participant can stop between tasks so may start at 10:00pm and end at 3:00pm 4 days later. Is that useful?
  24. Will look into better analysis, but we need to remember we are making plug-ins which will display data how they choose, itegration of data from multiple plug-ins will be different analysis plug-in. (don't want to hard code in UTS)
  25. Empty text on all lists/tables is on my TODO.
  26. Or you can select a single participant. Do we want to remove that feature? Later when graphs are used it may be nice to see data over several sessions?
  27. Mailing notifications and invites would be nice. I'll see how far I get.
Bevan’s picture

Bevan commented

Thanks for the answers and explanations. I have some points below. Everything else makes sense and my response is 'okay'.

  • #6: Try setting the variable destination in the URL or the form array.
  • #13 #14 #15 #18 #23 #26: I prefer we spend more time later rethinking and designing these UIs carefully rather than putting on band aids that provide marginal improvements on them now. Band aids just put-off the real solution even more.
  • #15: Yes, but it's also important the participant gets clear feedback when they've submitted a feedback item. Showing the last three entries and clearing the textarea when they submit (kinda like IM clients) does this better than a message saying 'submitted' or similar. It also allows them to maintain a flow of thought and pickup where they left off when they get distracted.
  • #18: Ah, that's the information that I was missing. That needs to be included in the UI urgently as the UTS is not usable without it.
  • #19: Quite possibly. Obviously the lack of feedback here is an issue.
  • #20: Hmmm, not sure if or how to make that optional. Can we always leave the database intact until the UTE clicks 'destroy' on the corresponding participant / session? In the future, we will likely need the ability to destroy some but not all datasets, since logs are tiny compared to streamed datasets, and a UTE is likely to want to retain logs and processed / mixed streams, but destroy raw streams.
  • #21: Isn't it as simple as implementing hook_form_alter() and storing everything that comes through? I'm probably missing something.
  • #23: Hmmm, good point. I think it's still useful, but perhaps we should publish the time the test finished instead, and probably order by this timestamp too. On the timeline we should include events where the participant stopped, took a break, and returned later. Are we sure we want 'Maximum time' to be a required field? I'm thinking there may be use cases where a time limit is not desirable?
  • #24: Yes, but all simple data capturers that have events can show those events on a timeline (or table of chronologically ordered events for now). Streamed data wouldn't show on the timeline (or table).
  • #26: I agree that in a richer data display it might be useful, but not in a 2-dimensional table.
  • #27: I agree it's a would-be-nice. It's probably a relatively simple feature with a large impact though.

Now that I was able to login to the test site I was able to generate these errors:

  1. When logging in and at other times, (possibly after submitting any form?) I get this drupal_set_message() error:
          user warning: Duplicate entry 'uts995079-1216855457' for key 1 query: INSERT INTO uts_path (session_id, timestamp, path, breadcrumb, title) VALUES ('uts995079', 1216855457, 'uts_proctor/task/2', 'Home', 'View task') in on line .
  2. After submitting live feedback, once I got this error as a javascript alert()-style popup. I was logged in as admin. I didn't get it when I subsequently tried again shortly after, still logged in as admin, but the task time limit had run out:
          The page at http://drupal6.lcl says:An error occurred. 
      /uts/data/live_text
      <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
        "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
      <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en" dir="ltr">
        <head>
          <title>drupal6.lcl</title>
          <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
      <link rel="alternate" type="application/rss+xml" title="drupal6.lcl RSS" href="http://drupal6.lcl/rss.xml" />

      <link rel="shortcut icon" href="/misc/favicon.ico" type="image/x-icon" />
          <link type="text/css" rel="stylesheet" media="all" href="/modules/node/node.css?x" />
      <link type="text/css" rel="stylesheet" media="all" href="/modules/system/defaults.css?x" />
      <link type="text/css" rel="stylesheet" media="all" href="/modules/system/system.css?x" />
      <link type="text/css" rel="stylesheet" media="all" href="/modules/system/system-menus.css?x" />
      <link type="text/css" rel="stylesheet" media="all" href="/modules/user/user.css?x" />
      <link type="text/css" rel="stylesheet" media="all" href="/themes/garland/style.css?x" />
      <link type="text/css" rel="stylesheet" media="print" href="/themes/garland/print.css?x" />
              <!--[if lt IE 7]>
            <link type="text/css" rel="stylesheet" media="all" href="/themes/garland/fix-ie.css" />    <![endif]-->
        </head>
        <body class="sidebar-left">

      <!-- Layout -->
        <div id="header-region" class="clear-block"></div>

          <div id="wrapper">
          <div id="container" class="clear-block">

            <div id="header">
              <div id="logo-floater">
              <h1><a href="/" title="drupal6.lcl"><img src="/themes/garland/logo.png" alt="drupal6.lcl" id="logo" /><span>drupal6.lcl</span></a></h1>        </div>


            </div> <!-- /header -->

                    <div id="sidebar-left" class="sidebar">
                          <div id="block-user-0" class="clear-block block block-user">

        <h2>User login</h2>

        <div class="content"><form action="/node?destination=node"  accept-charset="UTF-8" method="post" id="user-login-form">
      <div><div class="form-item" id="edit-name-wrapper">
       <label for="edit-name">Username: <span class="form-required" title="This field is required.">*</span></label>
       <input type="text" maxlength="60" name="name" id="edit-name" size="15" value="" class="form-text required" />
      </div>
      <div class="form-item" id="edit-pass-wrapper">
       <label for="edit-pass">Password: <span class="form-required" title="This field is required.">*</span></label>
       <input type="password" name="pass" id="edit-pass"  maxlength="60"  size="15"  class="form-text required" />
      </div>
      <input type="submit" name="op" id="edit-submit" value="Log in"  class="form-submit" />
      <div class="item-list"><ul><li class="first"><a href="/user/register" title="Create a new user account.">Create new account</a></li>
      <li class="last"><a href="/user/password" title="Request new password via e-mail.">Request new password</a></li>
      </ul></div><input type="hidden" name="form_build_id" id="form-c6c2883de224f93ac98d93a35947e1cf" value="form-c6c2883de224f93ac98d93a35947e1cf"  />
      <input type="hidden" name="form_id" id="edit-user-login-block" value="user_login_block"  />

      </div></form>
      </div>
      </div>
              </div>

            <div id="center"><div id="squeeze"><div class="right-corner"><div class="left-corner">
                                                                                                <div class="clear-block">
                  <div id="node-2" class="node">


        <h2><a href="/node/2" title="Create a homepage">Create a homepage</a></h2>

            <span class="submitted">Thu, 07/24/2008 - 11:23 — Bevan</span>

        <div class="content clear-block">
          <b>Maximum time:</b> 1 min<br /><p>Do it! Go on!</p>
      <p>Create a home page, I dare you!</p>
        </div>

        <div class="clear-block">
          <div class="meta">
              </div>

            </div>

      </div>
      <div id="node-1" class="node">


        <h2><a href="/node/1" title="Node forms">Node forms</a></h2>

            <span class="submitted">Thu, 07/24/2008 - 11:22 — Bevan</span>

        <div class="content clear-block">
          <b>Study status:</b> Active<br /><b>Study type:</b> Task Completion<br /><b>Audience:</b> End user (Beginner)<br /><b>Base environment:</b> Default installation<br /><b>Number of participants:</b> 10<br />  </div>

        <div class="clear-block">
          <div class="meta">
              </div>

            </div>

      </div>
                </div>
                <a href="http://drupal6.lcl/rss.xml" class="feed-icon"><img src="/misc/feed.png" alt="Syndicate content" title="drupal6.lcl RSS" width="16" height="16" /></a>          <div id="footer"><div id="block-system-0" class="clear-block block block-system">


        <div class="content"><a href="http://drupal.org"><img src="/misc/powered-blue-80x15.png" alt="Powered by Drupal, an open source content management system" title="Powered by Drupal, an open source content management system" width="80" height="15" /></a></div>
      </div>
      </div>
            </div></div></div></div> <!-- /.left-corner, /.right-corner, /#squeeze, /#center -->


          </div> <!-- /container -->
        </div>
      <!-- /layout -->

          </body>
      </html>
  3. Do we expect UTE's to account for time a participant may spend writing live feedback while completing a task? Do we want to pause the timer while the user types live feedback? This sounds really messy. Perhaps the timer needs to prompt the user to give up, rather than automoatically give up, end the task and expect the user to move on?
  4. Another new issue: On enabling UTS module (in a clean drupal6 site, without the required code additions in settings.php), user path and live text all at once, I get the following PHP error. We should deal with this case and give useful feedback instead of WSOD or ugly PHP error messages. This detailed output is thanks to xdebug module for PHP, but is normally a one-liner or WSOD. In any case it's meaningless to a significant portion of our audience, and annoying for the rest.
          ( ! ) Fatal error: Call to undefined function uts_session_active() in /Users/bevan/Sites/drupal6/sites/default/modules/uts/uts_proctor/uts_proctor.data.inc on line 63
      Call Stack
      #	Time	Function	Location
      1	0.0003	{main}( )	../index.php:0
      2	0.0033	drupal_bootstrap( )	../index.php:16
      3	0.0194	_drupal_bootstrap( )	../bootstrap.inc:933
      4	0.0277	_drupal_bootstrap_full( )	../bootstrap.inc:1022
      5	0.1029	module_invoke_all( )	../common.inc:2453
      6	0.1032	call_user_func_array ( )	../module.inc:471
      7	0.1032	uts_live_text_init( )	../module.inc:0
      8	0.1032	uts_proctor_data_record( )	../uts_live_text.module:120
boombatower’s picture

boombatower commented

Much of this has been postponed due to the bug with the menus disappearing. I have spend a while debugging that.

  • #15: Talked this over with Bohjan and looked at what several website do, it should turn out better, more like what your talking about. I just haven't gotten around to implementing all the JavaScript.
  • #20: You can destroy individual sessions with the current interface and I have change the code to leave the session database intact.
  • #21: Actually thought about that after I wrote it, so if you just want the data var_export() or serialized that would work.
  • #23: Not sure, could be a possibility. Lets think about that a bit and if we decide to make it not required I'll have to mess with the proctor code.
  • #27: I work on time granted, but I agree very useful.

Related to your errors:

  1. Already aware of and should be now fixed. (Just hadn't gotten to it, was busy debuging weird menu isse)
  2. Not sure exactly what happened, but a possible fix would be to have the page set to redirect when the task time limit is up that way the page will not display and of the plug-ins, but instead will show the next task or completed message. (working on adding completed message)
  3. Definitly a possibility to make it more like an optional parameter. Not sure how to decide, but I think trying to stop it when they type feedback is bad idea (as it will get messy). The maxtime doesn't have to be timed exactly like a race. If a task takes 5 minutes for an average user then set to 10-15 min. Intended to protect user from wasting all their time and energy, not be a speed test.
  4. To my understanding (Bojhan ran into same issue) that is a bug with core dependency checking. UTS doesn't get enabled, but the two plug-ins which depend on UTS do. UTS doesn't get enabled due to requirement. I haven't filled a bug report, but does that make sense. (That is what happened when I tried to re-create that) If so I will file bug report. I get the impress it isn't something the code was intended to deal with.
boombatower’s picture

boombatower commented

What would be a good way to display the login information through the UI? I added a settings page to allow configuration of the defaults, but I'm unsure of how to display them.

My original thought was the study/task description would contain the information since the base environment could contain several users that are used as part of the tasks.

boombatower’s picture

boombatower commented

Added some help text on environments page explaining login information.

Bojhan’s picture

Bojhan commented

Just wondering, do we really need to communicate the login information? Cant you just have the user logged in at the start of his session?

boombatower’s picture

boombatower commented

The only problem with that is say you want to test some anonymous features, or things involving multiple users. You may want to setup serveral at the beginning.

What you described would work for a large chunk if not almost all the situations, but I'm not sure we want to force that.

boombatower’s picture

boombatower commented

Initial mailing notifications and framework complete. Will add invites and further notifications in coming commits.

Bevan’s picture

Bevan commented

Responding to comment #6, Jimmy / boombatower:

  • #20: We want to be able to delete data feed types separately. e.g. delete video, but not the log or path. I think this approximately as important as been able to delete all data for an individual.
  • #21: serialized is more flexible than var_dump()

As for your other comments, 'okay'. It looks to me like you're on top of things -- Awesome!

The default base environment should ideally start the user at the last step of install.php, with the schema loaded but no users. I understand this may be difficult technically and possibly out of scope at this phase

Additional base environments should have a hook or property that provides a list of usernames, passwords and roles provided by the base environment. In the Study node/object we need to give the UTE the option of which of these users to display to the participant along with the study description, or with which task descriptions.

boombatower’s picture

boombatower commented

#20: will work on later.
#21: the recording of data is now implemented.

boombatower’s picture

boombatower commented
Assigned: Unassigned » boombatower
Status: Active » Fixed

I think everything discussed in this review was delt with in some manor. I have completed a number of things and have a few others on my TODO list.

Further reviews can be posted in a separate thread.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.