[Backend] Refactoring to best practices

[tsimica]

As discussed with @reidka, the Rails codebase needs to be refactored to adhere to best Rails practices. Some of the work would include (but not limited to):

General

• [ ] Bringing the codebase to use Rails 6. It's already being used in production at GitHub and Basecamp -- might as well upgrade to it.
• [ ] Gemfile can be refactored. Lots of unused gems can be removed. Gems that are being used should be versioned.
• [ ] Puma should be upgraded to 4.1. Perf testing should be done to know how many threads should be used in production.
• [ ] Styling should use 2 spaces, not sure why it was changed to 4. Controller classes should be Api::V1::ControllerName < ApplicationController. Follow Airbnb or Shopify conventions.
• [ ] Authorization is non-existent. Pundit (gem) can help here. This can be a separate issue.
• [ ] Background processing for emails & PDF generation. Consider Sidekiq or an alternative. Redis looks to be used in production but not development? Use it also in development to be consistent. You can use foreman to manage the processes running.

Rails Specific config/routes.rb

• [ ] Refactoring routes to match Rails conventions (e.g. "delete" is not a Rails verb, it's "destroy") app/controllers/concerns/Response.rb
• [ ] params are never needed to be casted to Integer, Rails does this automatically
• [ ] There are no status codes besides 200 returned by the api. render json: {...}, status: :bad_request should be used in render_error

app/controllers/api/v1/applications_controller.rb

• [ ] No use of before_action anywhere. invalid_id(Session, :session_id), Application.find(params[:id]) appear twice. These should be refactored.
• [ ] Using model.destroy! after a save is invalid raises serious concerns -- is this something that is explicitly needed? The database and model validations should enforce constraints.
• [ ] Update route should be a transaction -- either both succeed or both fail. If one succeeds but the other fails, it's treated as an error in the front-end but not in the back-end. This will lead to stale and inconsistent data.
• [ ] all_applications and application_data should be refactored into the model. If you're trying to include an associated model, consider using .includes then .to_json(includes: X)
• [ ] applications_by_session should also use ActiveRecord methods instead of Ruby methods
• [ ] Why use .exists? at all when .find can be used -- you get the object for free and can use the methods on it

Controllers not mentioned fall into some issue mentioned above, or they are yet to be posted here.

app/mailers/action_mailer.rb

• [ ] Consider using an environment variable for the email to be used

app/models/session.rb

• [ ] validates_uniqueness_of :name should be constrained by the database to avoid race conditions

[tsimica]

I had a look just now at https://github.com/uoft-tapp/tapp/pull/218/files. It's less idiomatic than master. I am not too what the intention was here.

[siefkenj]

Most of these items look good. Two comments: (a) we decided on 4 spaces over 2. This is a calculated deviation; (b) we decided that the API would aways return 200, unless there was an uncaught rails error. The returned JSON should include {status: "error", ...} if there was an error that was caught. (I'm not sure if this is what you were referring to, but how can this result in stale data?)

We are also deviating from traditional rails REST in a few key ways: we only use GET and POST, we always return JSON with status 200, and we have upsert instead of insert and update (the rule being if the ID is included, it is an update, otherwise an insert)

[tsimica]

Understood. Thanks for clarifying.

For the stale data part, consider the following:

• There are two updates happening in that route. Suppose one of them happens to fail and one succeeds. The database will not rollback the successful update unless it's wrapped in a transaction. The condition of sending a success when both are updated implies that you should be rolling back your changes when one fails. When you reload the page, or on the next AJAX request from React, there will be one model that will be inconsistent (assuming they get served to the UI).

[siefkenj]

I think we'll need to talk about this in person. There is something I am not understanding about this situation.

[siefkenj]

I assume this was handled by the rewrite.