Part of #1921152: META: Start providing tour tips for other core modules.

Problem/Motivation

Write tour integration for People admin page

Proposed resolution

Create tour yml files for required admin screens in admin/people.

Remaining tasks

Create patch for tour integration for primary People page

User interface changes

New tours

API changes

None

Technical pointers when creating tour tips

See: https://drupal.org/node/1921152#tour-tips-tech-note for tech notes on making tour tips.

Files: 
CommentFileSizeAuthor
#40 tour-people-2040845-40.patch2.39 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,013 pass(es).
[ View ]
#40 interdiff.txt583 byteshussainweb
#37 interdiff-2040845-35-37.txt1.11 KBjorgegc
#37 tour-people-2040845-37.patch2.39 KBjorgegc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,997 pass(es).
[ View ]
#35 tour-people-2040845-35.patch2.79 KBjorgegc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,032 pass(es).
[ View ]
#26 interdiff.txt1.8 KBpameeela
#26 tour-people-2040845-26.patch2.58 KBpameeela
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,413 pass(es).
[ View ]
#25 tour-people-2040845-25.interdiff.txt378 bytesnick_schuch
#25 tour-people-2040845-25.patch2.22 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 63,377 pass(es).
[ View ]

Comments

lisarex’s picture

StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 56,619 pass(es).
[ View ]

Content written and edited by colleagues :)

lisarex’s picture

Status:Active» Needs review
StatusFileSize
new2.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,150 pass(es).
[ View ]

Ignore previous. This patch contains two yml files

larowlan’s picture

Re the image, I don't think 'any site visitor' is true, I think 'approved users' would be more correct, as it's permission based.

nick_schuch’s picture

Issue tags:+Needs tests

Tour needs tests since we now have #2028535

Floydm’s picture

StatusFileSize
new2.03 KB
new3.93 KB
FAILED: [[SimpleTest]]: [MySQL] 58,844 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Updated the patch to add UserAdminTourTest.php and fix a couple of whitespace issues in the yml file.

One of the tests fails because it can't find an element with class .action-links, which the second tip is attached to. However I can see such an item in the verbose testing output:

<nav class="action-links">
<li>
<a class="button button-action" href="/d8/admin/people/create">Add user</a>
</li>
</nav>

If I attach the second tour tip to class "button-action" the test passes. Does SimpleTest not handle the html 5 nav element?

Status:Needs review» Needs work

The last submitted patch, tour-people-2040845-5.patch, failed testing.

lisarex’s picture

Assigned:lisarex» Unassigned

unassigning myself

Floydm’s picture

StatusFileSize
new2.37 KB
new5.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]

Updated patch that adds the test for the /user/*/edit tour. It also modifies the user admin tour test to no longer test for nav class="action-links", which I mentioned in comment #5 I could not get to pass and which I suspect has to do with it being an html5 nav element. Only a guess though.

Floydm’s picture

Status:Needs work» Needs review

Changing status to needs review.

Floydm’s picture

StatusFileSize
new3.13 KB
new2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es).
[ View ]

Just noticed issue #2044399: Write tour integration for user edit page exists for the /user/*/edit tour and that that tour appears more complete, so I am rerolling this patch to remove the tour and test for /user/*/edit.

nielsonm’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Works for me. Marking this as RTBC.

batigolix’s picture

Status:Reviewed & tested by the community» Needs review

The code may work but the content & wording needs a proper review first

nielsonm’s picture

Assigned:Unassigned» nielsonm
Status:Needs review» Needs work
Issue tags:+Tour Content Review

Needs content review and new router system based update

nick_schuch’s picture

Status:Needs work» Needs review
StatusFileSize
new3.09 KB
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] 63,678 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This patch updates the path to a route and updates the test coverage.

Status:Needs review» Needs work

The last submitted patch, 14: tour-people-2040845-14.patch, failed testing.

The last submitted patch, 14: tour-people-2040845-14.patch, failed testing.

nick_schuch’s picture

StatusFileSize
new389 bytes
new2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 63,404 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixes yml format issue.

nick_schuch’s picture

Status:Needs work» Needs review
nick_schuch’s picture

StatusFileSize
new2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 63,368 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new389 bytes

Reuploading for tests to run.

Status:Needs review» Needs work

The last submitted patch, 19: tour-people-2040845-17.patch, failed testing.

The last submitted patch, 17: tour-people-2040845-17.patch, failed testing.

nick_schuch’s picture

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tour-people-2040845-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 22: tour-people-2040845-22.patch, failed testing.

nielsonm’s picture

Assigned:nielsonm» Unassigned
nick_schuch’s picture

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 63,377 pass(es).
[ View ]
new378 bytes

Time for some green!

pameeela’s picture

StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,413 pass(es).
[ View ]
new1.8 KB

Updates content to better align with guidelines.

sun’s picture

This comment raises architectural design concerns and is not meant to discredit the work that went into this particular issue and patch.

+    label: 'Managing users'
+    body: 'View and edit user accounts.'
...
+    label: 'Add a user'
+    body: 'Create a new user account.'
...
+    label: 'Search for users'
+    body: 'Find users by applying filters.'
...
+    label: 'Update accounts'
+    body: 'Select one or more users via the checkboxes to apply changes to the accounts.'
...
+    label: 'Edit user account'
+    body: 'Make changes to one user account.'

Hrm. — Do we really want to explain every possible UI interaction in tour tips?

If we do, what kind of "tip" or explanation is "Create a new user account." meant to give that wouldn't be obvious by the UI already?

I was under the assumption we introduced Tour tips to explain non-obvious things in the user interface only?

→ If we need to provide hints for the most fundamental/basic operations, then our UI/UX must be seriously flawed? (which I don't think it is)

I'm afraid, this makes little sense to me. These kind of Tour tips will only make people avoid Tour module whenever possible.

Introducing any kind of such "tips" presents a major disrespect and discredit for the hard work of Drupal's Usability and Design teams, because the mere fact of pointing out the most basic user interface operations via Tour tips would mean that the entire user interface design is a utter #FAIL.

Or to put that in reverse, because it might not be obvious:

The user interface design is solely responsible for making obvious things obvious. User interface design enables end-users to use Drupal intuitively. A well-designed user interface needs absolutely zero explanation; every human immediately understands how to use it. That is the one and only goal of user interface design.

This patch (and likely others) introduces tips for the most obvious operations in the user interface. These operations should not need any kind of explanation in the first place. IF they do, then the solution is NOT to explain to the user how to use the interface. — The only acceptable solution is to fix the user interface design.

In short, if there is a problem with any of these basic operations, then we need to fix the UI. Tour tips should be reserved for non-obvious interactions.

A non-obvious interaction would be to point out that it is possible to add fields to users. Or to add view modes. Or that you can change the administrative user listing/view to customize it to your needs. Or that you can leverage the "Reset password" functionality to temporarily grant someone else access to a user account without sharing the password. — All of that is not obvious, and cannot be solved by user interface design.

batigolix’s picture

Some background discussion about the role of tours can be found in #1921152: META: Start providing tour tips for other core modules.

pameeela’s picture

sun, I could not possibly agree more. I think there has been some motivation to get more tours in and that has resulted in trying to create ones for a lot of admin pages that don't really need them. Or maybe this page should have one but as you said, it should point out the less obvious things.

There should probably be some more discussion about this sooner than later, to avoid unnecessary work.

rodrigoaguilera’s picture

Status:Needs review» Needs work

The routes section is separated in two lines.

the test need annotation to be detected as such instead of getInfo() and at least a setUp function and some actual tests.
Is as simple as the following example

<?php
 
/**
   * {@inheritdoc}
   */
 
protected function setUp() {
   
parent::setUp();
   
$this->adminUser = $this->drupalCreateUser(array('administer content translation', 'access tour'));
   
$this->drupalLogin($this->adminUser);
  }

 
/**
   * Tests Content translation tour tip availability.
   */
 
public function testContentTranslationTour() {
   
$this->drupalGet('admin/config/regional/content-language');
   
$this->assertTourTips();
  }
?>
rodrigoaguilera’s picture

Issue tags:+Novice

marking it as novice since is a task that can teach a lot about the drupal system

jorgegc’s picture

I am happy to work on this one guys.

jorgegc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,032 pass(es).
[ View ]

Uploading patch for review. Please let me know if there is anything else that we need to test. I've basically looked at what we are doing in Forum and replicated it.

benjy’s picture

Status:Needs review» Needs work
  1. +++ b/core/modules/user/src/Tests/UserAdminTourTest.php
    @@ -0,0 +1,61 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function setUp() {
    +    parent::setUp();
    +    $this->adminUser = $this->drupalCreateUser(array('administer users', 'access tour'));
    +    $this->drupalLogin($this->adminUser);
    +  }

    Remove this, the user should be created in the parent using the permissions from $permissions.

  2. +++ b/core/modules/user/src/Tests/UserAdminTourTest.php
    @@ -0,0 +1,61 @@
    +  /**
    +   * Tests user list tour tip availability.
    +   */
    +  public function testUserListTour() {
    +    $this->drupalGet('admin/people');
    +    $this->assertTourTips();
    +  }

    This isn't needed, see TourTestBasic::testTips().

jorgegc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,997 pass(es).
[ View ]
new1.11 KB

Changes as per comments in #36.

benjy’s picture

Status:Needs review» Reviewed & tested by the community

Looks great. RTBC if we're green.

cilefen’s picture

+++ b/core/modules/user/src/Tests/UserAdminTourTest.php
@@ -0,0 +1,42 @@
+   * The permissions required for a logged in user to test tour tips.

This is a total nit, but logged-in is a compound adjective and it should be hyphenated. You will find the wrong way in the same file as the right way in core, like ForumTest.

hussainweb’s picture

Issue tags:+DrupalSouth
StatusFileSize
new583 bytes
new2.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,013 pass(es).
[ View ]

I see http://english.stackexchange.com/a/11002 which also points to the same suggestion. Changing it. :)

I think this can still be RTBC.

jorgegc’s picture

Thanks @hussainweb :-)

webchick’s picture

Status:Reviewed & tested by the community» Postponed

Thanks a lot for your work on this issue! I reviewed it and it seems to work well.

I do have some concerns, however:

#1: The page in question is a View. In this respect it's identical to admin/content, admin/comments, etc. but those pages do not (yet) have Tours. Therefore, committing this patch would introduce an inconsistency with other, similar admin pages.

#2: I also share some of sun/pameela's concerns from #27/#29. We need to come to agreement on guidelines of when we do/do not use Tours for certain admin interactions.

Therefore, very sorry to do this, but I think we need to mark this postponed on figuring out such guidelines up in the parent issue at #1921152: META: Start providing tour tips for other core modules..

Since you've done great work on this and had the opportunity to play with Tour module, please chime in there with your thoughts!