-
Notifications
You must be signed in to change notification settings - Fork 58
Mass Add Feature #6
Comments
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. |
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. |
Notes:
|
|
…rs that had conflicted in the first attempt
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 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/models/user.rb Line 129: app/controllers/users_controller.rb Line 123: Using unless in this way is confusing. I feel like 'if !condition' is more readable: or even better, I think this should work: Other stuff: I feel like we can refactor the two User class methods a bit. Let's talk about this together. |
Idea for rest of refactoring:
|
(had accidentally closed) |
@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. :-) |
@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:
|
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). |
@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. :-) |
I'd like to be able to add batches of people (i.e. new STs/new BMTs). Ideally, it would be something where I could pull things from our spreadsheets/databases and enter everyone on a separate line with some sort of ability to enter affiliation and phone number as well (every bit of data separated by a "|" or an asterisk, etc.). Should only be able to add one kind of role at a time; when the mass add is taking place, they will all be normal, admins, or checkout person, or banned.
The text was updated successfully, but these errors were encountered: