Import CSV breaks

[shippy]

In Users view > Import Users from CSV (departments/#{dept_id}/users/import), hitting Upload (with or without file) leads to error. (Reported by @stuartteal )

[shippy]

The error here is No route matches [POST] "/delayed_job".

The form submission goes to the verify_import action. This resolves to /delayed_job?action=verify_import&controller=users - that is, Users#verify_import is fed through DelayedJobWeb, a delay process.

It's unclear to me how url_for knows to feed the import process through DelayedJobWeb - a quick search doesn't reveal any config file. Will hunker down with the docs for delayed_job and url_for.

I suspect that whatever made the mailer -- the other main feature that uses delays -- work, broke this functionality. I'll review #449, #450, #451 and #453 to see if any of those changes could be responsible.

[shippy]

Thought: a straightforward fix would be to bypass the job delay and point the form directly to Users#verify_import. This would be a reasonable interim fix, but there is (likely) a reason why we're using a delay for the batch import in the first place?

[caseywatts]

I bet it's only delayed b/c it takes a long time? As long as admins don't mind waiting for a long pageload, I bet you can remove its dependency on delayed_job and make it simpler :D

(but btw I didn't write this code, Adam probably did - I'm just postulating)

[shippy]

Still, this is confusing. There's clearly a line in routes.rb: match "/delayed_job" => DelayedJobWeb, anchor: false. rake routes registers it. I don't see why it wouldn't be matched.

@caseywatts The problem is that, as far as I can tell, the task never reaches the jobs pipeline. There's a 404 in the process.

[caseywatts]

Maybeee in Rails 4 match means get by default? The railsguide isn't clear what match without via would do.

If this is only ever post you could say post instead of match, or maybe just add , via: [:get, :post] to the end?

[shippy]

Adding the via doesn't fix the problem. (Changed routes.rb, restarted rails s, tried again and failed.)

[shippy]

Further attempts:

• Tried adding :put to the via; no luck.
• GET /delayed_job shows the DelayedJobWeb dashboard.
• Specifying the controller doesn't change the routing. Specifying a different csv action in the same controller - say, save_import - did.

[caseywatts]

you've tried put, did you try post?

[shippy]

OH GOD OF COURSE. This action is not intended to be a delayed job. The reason why url_for(action: 'verify_import') pointed to /delayed_job is that *routes.rb does not have a specified verify_import action. The last line of routes.rb, however, with anchor: false, catches all uncaught routes so far.

The solution is to add post :verify_import to routes.rb.

The import still breaks, however, and the terribleness of rescue Exception => e is obscuring why. Investigating undefined method 'department' for nil:NilClass now - it is very likely in app/models/user.rb#self.import(file).

[caseywatts]

yesssss progresss :D :tada:

[shippy]

Okay, two different errors with two different reasons:

1. Obtaining a CSV via export feature, with the seeded data, results in a file with no roles. The import procedure cannot handle this. This is where undefined method 'department' for nil:NilClass comes from - it needs a role to specify the user's department: u.departments = [roles.first.department]. I will assume for now that this users must ordinarily have a role, and that the role determines the department, and will not fix this.
2. Getting a real-life CSV file from @stuartteal, a different problem emerges: his headers are "human-readable" -- e.g. Login, First Name, whereas the script assumes they will be "db-readable" -- e.g. 'login', 'first_name'. This results in unknown attribute: Login on line u = User.new(attrs). This should be a trivial fix - I just need to dig up the Rails function that takes care of exactly this translation. On it now.

[shippy]

Okay. Upon re-reading the code, we don't have:

1. a specification of how the CSV should be formatted or what the values can be,
2. a specification of treatment when some values - say, roles - are missing,
3. a specification of treatment when some values - say, roles - are non-standard.

This is mostly a problem in the treatment of roles. As far as I can tell, the code expects that the CSV file will encode an array of roles *, at least one of which will have been defined in the Shifts instance it is being imported into. This is silly in a number of ways. Instead, I propose the following standard:

1. Roles are mandatory because each user has to have a department (as User model has validate :departments_not_empty). A department, at least at the import stage, is defined by role.
2. Roles will be listed either in column named roles or role.
3. If the user has multiple roles, they will be separated by a semicolon on the field. User's department affiliation will be defined by his first role's department affiliation.
4. Each role listed by name must be defined in the current Shifts instance. (You cannot import a "CT" if the "CT" role is as of yet undefined.)

The expected behavior if either of these conditions isn't met is (a) a failure to create new user, (b) a clear notification of the way in which the record failed to meet expectations.

Undefined cases: What should the import do if a user would be duplicated? How should the results of the import be reported?

(* How it might encode an array of roles is unclear - best as I can tell, there is no mechanism in which the Ruby CSV library would automatically expand a particular value into an array. It's not that it is difficult - it's that the current code just doesn't account for the case.)

[shippy]

Rolled PR #473 that fixes the initial error, entails sensible error reporting, and implements the specifications above. This issue will close when the PR is reviewed and merged.

[shippy]

On discussion with Stuart, striking number 3 (multiple roles) from the spec. Will refactor and merge.