Moodle
  1. Moodle
  2. MDL-29244

Make 'selecting users who have never logged in' functionality more user-friendly

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1
    • Fix Version/s: 2.2
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      1. Log in as admin
      2. Create 2 users who have never logged in (if not already present)
      3. Go to user bulk action page (Site administration -> Users -> Bulk user actions)
      4. Click "Show advanced"
      5. Select "Never Accessed" and click "Add Filter"
      6. Make sure the "Never Accessed" filter is visible and "Users" should contain both the users (which were created in step 2)
      7. Click on "Remove all filters"
      8. Repeat step 4 - 7, witch "Never modified" option
      9. Repeat step 4 - 7, witch "Never modified" and "Never accessed" option
      10. Login as user1 and logout
      11. Log in as admin and repeat 1-9, in case of "Never modified" both the users should be visible and in case of "Never accessed" only one user2 should be visible
      12. Log in as another user and "Edit profile" (Settings -> My profile settings -> Edit profile)
      13. Log in as admin and repeat 1-9, in case of "Never modified" user1 should be visible and in case of "Never accessed" user2 should be visible and in "Never accessed" and "Never modified" neither one should be visible
      14. Assign user1 as teacher of course and repeat step 3 and 4
      15. In "Course role" select Teacher and click Add filter
      16. user1 should be visible
      17. click "Remove all filters" and repeat 14 and 15 with specific category and make sure right user is visible.
      Note:
      Make sure to check it on 20 branch.

      Show
      1. Log in as admin 2. Create 2 users who have never logged in (if not already present) 3. Go to user bulk action page (Site administration -> Users -> Bulk user actions) 4. Click "Show advanced" 5. Select "Never Accessed" and click "Add Filter" 6. Make sure the "Never Accessed" filter is visible and "Users" should contain both the users (which were created in step 2) 7. Click on "Remove all filters" 8. Repeat step 4 - 7, witch "Never modified" option 9. Repeat step 4 - 7, witch "Never modified" and "Never accessed" option 10. Login as user1 and logout 11. Log in as admin and repeat 1-9, in case of "Never modified" both the users should be visible and in case of "Never accessed" only one user2 should be visible 12. Log in as another user and "Edit profile" (Settings -> My profile settings -> Edit profile) 13. Log in as admin and repeat 1-9, in case of "Never modified" user1 should be visible and in case of "Never accessed" user2 should be visible and in "Never accessed" and "Never modified" neither one should be visible 14. Assign user1 as teacher of course and repeat step 3 and 4 15. In "Course role" select Teacher and click Add filter 16. user1 should be visible 17. click "Remove all filters" and repeat 14 and 15 with specific category and make sure right user is visible. Note: Make sure to check it on 20 branch.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-29244
    • Rank:
      18787

      Description

      Following on from MDL-15502, it seems the 'Never included' checkboxes in bulk user actions are difficult to understand.
      http://tracker.moodle.org/secure/EditIssue!default.jspa?id=47351
      How about removing the 'Never included' checkboxes and instead having the following options:

      First access	[] is after 
      		[] is before
      
      Last access	[] is after
      		[] is before
      
      Never accessed	[]
      
      Last modified	[] is after
      		[] is before

      I suggest we also remove the Last login options as they are very similar to the last access options. Last login is when a user last logged in, last access is the last time some activity on the site was logged, so it seems unlikely that these dates would differ by more than 1 day. Thanks to Rajesh for explaining these options.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Bringing this into the STABLE backlog as it is related to work in a current sprint.

          Show
          Michael de Raadt added a comment - Bringing this into the STABLE backlog as it is related to work in a current sprint.
          Hide
          Rajesh Taneja added a comment -

          Access filter should behave like:
          "First access" AND "Last access" AND "Last modified" OR "Never accessed"
          Anding "Never accessed" with "First access" and "Last access", won't make any sense as user will not get any results.

          Show
          Rajesh Taneja added a comment - Access filter should behave like: "First access" AND "Last access" AND "Last modified" OR "Never accessed" Anding "Never accessed" with "First access" and "Last access", won't make any sense as user will not get any results.
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Just been looking at this patch and have picked up the following things within the new checkbox.php. I'm sure most of them will have just been carried over from copy/paste and what not however given its a new file it'd be nice to get them all cleaned up.

          • Missing boilerplate
          • Needs the check to prevent direct access, check the top of lib/moodlib.php
          • methods should be properly scoped (public, protected, private)
          • PHPdocs for the disabled if and field properties need improvement (@var)
          • Reference handling within $mform should be removed (unneeded PHP passes all objects by ref)
          • Whitespace issues on the following lines: 26, 29, 73, 84

          In regards to the changes in general I think they are fine - the only thing I would consider doing is moving the new checkboxes below the date selectors for the related fields. e.g. First accessed, last accessed, never accesses - and - last modified, never modified.

          Other than that spot on.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Just been looking at this patch and have picked up the following things within the new checkbox.php. I'm sure most of them will have just been carried over from copy/paste and what not however given its a new file it'd be nice to get them all cleaned up. Missing boilerplate Needs the check to prevent direct access, check the top of lib/moodlib.php methods should be properly scoped (public, protected, private) PHPdocs for the disabled if and field properties need improvement (@var) Reference handling within $mform should be removed (unneeded PHP passes all objects by ref) Whitespace issues on the following lines: 26, 29, 73, 84 In regards to the changes in general I think they are fine - the only thing I would consider doing is moving the new checkboxes below the date selectors for the related fields. e.g. First accessed, last accessed, never accesses - and - last modified, never modified. Other than that spot on. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Sam
          Yeah, it is copy/paste thing.. I was about to change that before pushing it for integration, but forgot to mention in my comments

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Sam Yeah, it is copy/paste thing.. I was about to change that before pushing it for integration, but forgot to mention in my comments
          Hide
          Rajesh Taneja added a comment -

          Hello Sam,

          Can you please review this again. I have updated the branch with your feedback.
          One question:
          Do we need to create another issue, to fix direct access issue in user/filter/*.php ?

          Show
          Rajesh Taneja added a comment - Hello Sam, Can you please review this again. I have updated the branch with your feedback. One question: Do we need to create another issue, to fix direct access issue in user/filter/*.php ?
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Had a look at it again and noted the following all within checkbox.php again:

          1. Comments for the properties should be phpdocs still, e.g. for disableif /** @var array list of all the fields which needs to be disabled, if checkbox is checked */
          2. Line 56 indentation
          3. Line 100 whitespace of array

          Other than that spot on, feel free to put it up for integration upon fixing those up.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Had a look at it again and noted the following all within checkbox.php again: Comments for the properties should be phpdocs still, e.g. for disableif /** @var array list of all the fields which needs to be disabled, if checkbox is checked */ Line 56 indentation Line 100 whitespace of array Other than that spot on, feel free to put it up for integration upon fixing those up. Cheers Sam
          Hide
          Petr Škoda added a comment -

          please do not delete strings from stable branches, it would break non-english lang packs

          Show
          Petr Škoda added a comment - please do not delete strings from stable branches, it would break non-english lang packs
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr, strings are back in stable branches.

          Show
          Rajesh Taneja added a comment - Thanks Petr, strings are back in stable branches.
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          just looking at this within user/filters/checkbox.php :

              /**
               * list of all the fields which needs to be disabled, if checkbox is checked
               * @var array
               */
              protected $disableif = array();
          ...
           @param array $disableif name of fields which should be disabled if this checkbox is checked.
          ...
          foreach ($this->disableif as $disableif) {
          

          although minor, the $disableif variable really doesn't describe what it holds for me. would you mind renaming it to something more descriptive? perhaps $disabledfields ?

          Show
          Aparup Banerjee added a comment - Hi Raj, just looking at this within user/filters/checkbox.php : /** * list of all the fields which needs to be disabled, if checkbox is checked * @ var array */ protected $disableif = array(); ... @param array $disableif name of fields which should be disabled if this checkbox is checked. ... foreach ($ this ->disableif as $disableif) { although minor, the $disableif variable really doesn't describe what it holds for me. would you mind renaming it to something more descriptive? perhaps $disabledfields ?
          Hide
          Rajesh Taneja added a comment -

          Thanks Aparup,

          It's done, Have renamed it to disableelements

          Show
          Rajesh Taneja added a comment - Thanks Aparup, It's done, Have renamed it to disableelements
          Hide
          Aparup Banerjee added a comment -

          looks like nice work , this has been integrated. To master only. I'm not sure about the stable branches so i've kept it out to be safe. (Rajesh wasn't sure either). This was also classified as an improvement.

          Please open a separate linked issue where patches can be aimed towards a fix for stable branches if needed.

          ps:
          This has been a very difficult issue to follow. Very little was explained within this issue itself.

          Please ensure that linked issues's statuses are updated accordingly. (MDL-27673 is deferred. perhaps a 'deferred' should have an issue link?)

          Test has a typo : witch -> with/which ?
          Also i didn't run through the test but please ensure that test described in MDL-27673 is covered with the test here.

          Show
          Aparup Banerjee added a comment - looks like nice work , this has been integrated. To master only. I'm not sure about the stable branches so i've kept it out to be safe. (Rajesh wasn't sure either). This was also classified as an improvement. Please open a separate linked issue where patches can be aimed towards a fix for stable branches if needed. ps: This has been a very difficult issue to follow. Very little was explained within this issue itself. Please ensure that linked issues's statuses are updated accordingly. ( MDL-27673 is deferred. perhaps a 'deferred' should have an issue link?) Test has a typo : witch -> with/which ? Also i didn't run through the test but please ensure that test described in MDL-27673 is covered with the test here.
          Hide
          Rajesh Taneja added a comment -

          I feel this is more a Bug then improvement, as previous implementation was not creating the correct filter and "Not included" was misleading.

          Show
          Rajesh Taneja added a comment - I feel this is more a Bug then improvement, as previous implementation was not creating the correct filter and "Not included" was misleading.
          Hide
          Helen Foster added a comment -

          +1 for adding this to stable branches since, as Rajesh mentions, the issue could be considered a bug.

          Show
          Helen Foster added a comment - +1 for adding this to stable branches since, as Rajesh mentions, the issue could be considered a bug.
          Hide
          Markus Kemmerling added a comment -

          +1 from me too, since it also fixes bug MDL-27673.

          Show
          Markus Kemmerling added a comment - +1 from me too, since it also fixes bug MDL-27673 .
          Hide
          Andrew Davis added a comment -

          Testing in master pasted although Ive raised MDL-29543. Suspect that bug was always there.

          Ultimately its Aparup's decision whether or not this goes into 2.0 stable. I suspect it could go in as it doesnt involve an database changes and the existing filter system is pretty busted.

          Show
          Andrew Davis added a comment - Testing in master pasted although Ive raised MDL-29543 . Suspect that bug was always there. Ultimately its Aparup's decision whether or not this goes into 2.0 stable. I suspect it could go in as it doesnt involve an database changes and the existing filter system is pretty busted.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My +1 goes to backport this to STABLE branches as far as it's sort of improv/bugfix.

          But doing that in separate ("Backport MDL-29244 ...") issue next week, if no further problems are found here. Just in case it needs separate discussion / agreement / whatever.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - My +1 goes to backport this to STABLE branches as far as it's sort of improv/bugfix. But doing that in separate ("Backport MDL-29244 ...") issue next week, if no further problems are found here. Just in case it needs separate discussion / agreement / whatever. Ciao
          Hide
          Aparup Banerjee added a comment -

          Thanks for +1's! note that MDL-29541 has been created for backporting into stable.

          Show
          Aparup Banerjee added a comment - Thanks for +1's! note that MDL-29541 has been created for backporting into stable.
          Hide
          Aparup Banerjee added a comment -

          fixes have been rolled merrily up the stream! Thanks everybody!

          Show
          Aparup Banerjee added a comment - fixes have been rolled merrily up the stream! Thanks everybody!

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: