Feature #2681

E-mail notification when a new user hits the "Inactive user" page.

Added by Ward Vandewege over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
05/06/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
0.5

Subtasks

Task #2741: code review of 2681-new-inactive-user-notification branchResolvedTom Clegg

Task #2740: implementResolvedWard Vandewege

Associated revisions

Revision f6235f6b
Added by Ward Vandewege over 6 years ago

Merge branch '2681-new-inactive-user-notification'

refs #2681

Revision ab2550bc (diff)
Added by Ward Vandewege over 6 years ago

Fix workbench URL in inactive user notification e-mail.

refs #2681

History

#1 Updated by Ward Vandewege over 6 years ago

  • Assigned To set to Ward Vandewege

#2 Updated by Tom Clegg over 6 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-05-28 Pipeline Factory

#3 Updated by Tom Clegg over 6 years ago

Notes
  • Document how to specify multiple recipients. If array is acceptable, perhaps empty/single should be given as [] and ['foo@example.com'] respectively?
  • In the notification email, include a link to the appropriate users#show page, or at least users#index, so it's easy to click through to see details & activate.
  • Use the set_user_from_auth :admin helper instead of knowing how to manipulate Thread.current. (Maybe even update all the existing uses of Thread.current in user tests, so people stop copying them.)
  • The tests seem to amount to "try all four permutations of A-on/off and B-on/off" which would probably be nicer to do with two nested loops with one test inside, rather than all the copying and pasting. But this works.
  • A bit weird that the configuration settings don't get put back to normal after the tests run, although in this case I don't imagine it will actually cause any grief so probably not worth worrying about.

#4 Updated by Ward Vandewege over 6 years ago

  • Status changed from New to In Progress

#5 Updated by Tom Clegg over 6 years ago

  • Target version changed from 2014-05-28 Pipeline Factory to 2014-06-17 Curating and Crunch

#6 Updated by Tom Clegg over 6 years ago

At 4609a76

This isn't reliable. According to "rake test" on my system, you also can't depend on not having system_user in your database at the beginning of the test.

    # verify there is one extra user in the db now
    # the API server also auto-creates the root system user after the first user
    # is created, hence the test for the delta of 2.
    assert_equal @all_users.size+2, User.find(:all).size

I thought db/seeds.rb took care of this properly so we didn't need this sort of hack. Ideally we can figure out why this isn't sufficient on your system.

Failing that, we used to have a better workaround (call "system_user" in a setup block to make sure it exists) but you seem to have removed it (by accident?) in e9fc734 -- perhaps it needs to be put back?

#7 Updated by Ward Vandewege over 6 years ago

Thanks for that - I reinstated the loading of system_user which allowed me to go back to +1. Have another look?

#8 Updated by Tom Clegg over 6 years ago

Looks great

#9 Updated by Ward Vandewege over 6 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF