Mass Add Feature

[yalestc]

Mass Add Feature

[dgoerger]

I think we should do this using a separate controller and table in the database, so we can store "temporary users" somewhere for data review / before we actually submit to save to the "real" database, similar to what the valiant @ebmaher has done with the cart not-yet-finalized reservations.

[dgoerger]

Okay, nevermind Erin you didn't go the controller route, this can also work well with a separate controller action in Users, which I'll then work on.

[dgoerger]

Notes:

• modify to work with ALL the data columns, in case LDAP undefined / unavailable
• give preference to CSV file
• give a clearer example of CSV in the view prior to file upload

[dgoerger]

• option when you submit to overwrite on netid conflict (a sort of 'update all the fields' button)
• option on success page to reimport conflict users (which are saved in a hash, so would be trivial to resubmit and save)
• delete all just-imported option (in case you REALLY screwed up) adam said nvm about this
• change the deactivate buttons to hard-delete buttons, since that won't break anything and probs would not have imported if you're just going to deactivate / not want to wholly delete, and in case there was a problem with just one record actually I think we don't want this

[adambray]

Code review:

In general, make your variable names more clear. For example, there are lots of cases where you use an instance variable called 'user' where it's really a user_id. This makes code much easier to read. Another example, @user_formatted should
be @formatted_user most likely.

Also, in the import controller action, I feel like there are a few cases where you're using instance variables (ie @user) where you can just use local variables (user), i.e. line 142.

Other stuff:
app/controllers/application_controller.rb Line 186:
Can this be simplified using Rails' 'to_sentence' method?

app/models/user.rb Line 129:
Can be simplified to:
https://gist.github.com/3183717

app/controllers/users_controller.rb
Line 116 - 120 can be shortened to:
@overwrite = (params[:overwrite] == '1'))

Line 123: Using unless in this way is confusing. I feel like 'if !condition' is more readable:
if !file.nil?
...stuff

or even better, I think this should work:
if file
#do stuff

Other stuff:
CSV import should be limited to admins I think.
You're only enforcing this at the view level. The import controller action should check for admin status as well, and redirect if it's not present.

I feel like we can refactor the two User class methods a bit. Let's talk about this together.

[dgoerger]

• renamed unclear variables
• made variables local when appropriate
• to_sentence might simplify, might not; need to test to see how much shorter or cleaner it would be.
• user.rb ln129 not really simplifiable unless I extracted it to elsewhere
• simplified @overwrite and if file
• restricted to admin

Idea for rest of refactoring:

Controller: get file

Model: Parse file, with status codes, already having done LDAP if necessary. Read first line headers as symbols for hash

# see if CSV lib can parse to hash; otherwise put in module

[{:login => ' ', :first_name => ' '}] # for example

Model: Create all of the users which can be created.

{:success => [[user_object,'Successfully created.'],[user_object,'Successfully updated.']],
 :fail => [[user_hash,''],[user_hash,'update']} # the error messages

Controller: Prep for display

View: Display

[dgoerger]

(had accidentally closed)

[dgoerger]

http://stackoverflow.com/questions/2609891/get-in-that-db-parsing-csv-using-ruby?rq=1 <-- this one works

[dgoerger]

@adambray I've still got some very-regrettable code duplication in the users model (lines 156-169 and 196-209), but I don't see a particularly elegant way around it. I could extract that code block down out of the big (if update_existing)..else statement, and then move the sub-else's below it; but then I'd have to duplicate the update_existing-if and user declaration statements. :-(

Otherwise I think this is a much cleaner implementation. :-)

[dgoerger]

@adambray I've implemented the changes we talked about on Friday ('location' variable => 'filepath'; safe import defaults in case undefined [for 'do not update existing' and 'user_type = normal']; and modularized the duplicate code in user.rb as far as possible [still some dup in error handling, but not really avoidable]).

This implementation still relies on the first line giving the header / column information, though. When you have time, I'd be interested to hear your idea for handling when the first line does NOT give proper header information. Right now I have an (uncommitted) test in the users controller action:

imported_users.first.keys.each do |key|
  unless current_user.attributes.symbolize_keys.keys.include?(key)
    flash[:error] = 'Unable to import CSV file. Please ensure the first line of the file includes proper header information (login,first_name,...) as indicated below.'
    redirect_to :back and return
  end
end

[dgoerger]

re above: what I don't like about that solution is that it (1) calls current_user too many times (fixed on my local) and (2) will fail if you have an extra column of data that the controller has no use for (I tested by adding the extraneous column 'potato', for example).

[dgoerger]

@adambray That test (code posted two days ago in this issue; except before loop declare user_attributes_keys = current_user.attributes.symbolize_keys.keys [an array] to avoid unnecessary calls to current_user in the loop) could work well even in the second case (extraneous columns), if the flash[:error] were updated to indicate please no extraneous columns.

That is, unless we come up with a better idea; although I would like to see some form of CSV import in v3.1, so, perhaps this code (with or without this test) is good enough for the time being? Of course am open to suggestions. :-)