crawfoal / metanoia Goto Github PK

Give instructions on how to run through the demo, and only link to the production version (don't give staging link).

Since it sends one request per climb, this could take a long time and annoy the users.

There a lot of places where I'm doing this:

Climb.color_name_for(climb.color)

You can only see this in production. When you try to add a new employee (or give an existing one a new role), it doesn't work, saying that the role isn't in the list.

I think what is happening is that the code is eager loaded in production, and EmploymentForm is loaded before the role story classes, so when this validation is defined:

validates :role_name, inclusion: { in: Employment.roles.map(&:to_s) }

Employment.roles hasn't be set up properly yet.

It should render the flash and put it before main.

This would be a good time to move the flash partial to the shared folder (it is currently in the layouts folder).

See climb log links, for a white climb

Admins should be allowed to get a gym set up, but after it is "published", they should no longer be able to edit the gym or it's employees. It should be required that before a gym is published there is at least one manager assigned.

Choose a more vibrant color scheme. Also, I'd like to pick one main color for each role, and have the color switch when the role switches so that it is more obvious to the user what their current role is.

Don't merge the employment errors directly into the form errors. Keep them separate, and instead create an instance variable to hold the merged errors. To perform the merging, use ActiveModel::Errors#copy! (which is available in Rails version 5.0.0.1) to copy the form errors, and then merge in the employment errors as I was doing.

Review the README at guard-rspec. For example,

• definitely want to have a special command for when all specs are running (to make sure that test coverage is turned on)
• consider setting failed_mode to focus, and having all unit tests listed first
• maybe set all_after_pass to true

Also, maybe define different groups (could separate out unit and integration tests).

Also, install guard-spring

This would also be a good opportunity to do a general code review while the application is still fairly small.

After a user switches from admin to athlete, the current_user method seems to be returning nil. Try logging in as admin1, switching to athlete role, then clicking on Gyms.

• make form more intuitive
• should find row with employee, and add the new role name (current it just adds a new row, so the employee is listed twice, once for existing roles, once for new role)

E.g. a setter at one gym shouldn't be able to modify stuff at another gym.

In The Four Phases of Testing, they describe why it is important to only have one set of the four phases in each test.

Review the feature specs and try to apply this as much as possible.

Factories can be for non-active record objects too.

If a complex set up is needed in more than one place, consider if a (rather complicated) factory is appropriate.

Since there aren't any redirects, the flashes don't cleared out. We'll have to do it manually.

The climb histograms aren't resizing when I resize the browser window. They are responsive in that the size adjusts to the size of the window at the time that the page is loading, so it's not a huge issue, but I'd still like to fix it. Maybe use a Bourbon Neat grid instead of flexbox.

Better to use User#employed_at?, but we'll need to adjust that method so it can be called without a specific role in order to see if a user is employed at a given gym with any role.

Search bots can click on link_to'sThese links won't work if there is no logged in, but it still good to change them.

Make a separate #roles_in_words method, and have #roles just returns the array of strings / symbols. We don't always need to turn it into a sentence, e.g.

# app/policies/employment_policy
def index?
  Employee.new(email: user.email, gym_id: employment.gym_id).roles.present?
end

• hold information about the role story models and tables (basically all of the role story and role name related stuff that is currently in the Employment model)
• have a method for_user that behaves like an ActiveRecord scope, returning all role stories for a particular user (something like SetterStory.where(user: user).or(ManagerStory.where(user: user) only with metaprogramming so that all types of role stories are handled; maybe define a for_user method for each role story model in the Employable concern

Might be tempting to call it RoleStories because it contains information about all of the role story models, however if we call it RoleStory, then calls to the method above will look kind of like regular ActiveRecord stuff.

Check to see that each validation has a matching db constraint and vice versa.

Things would probably be cleaner if some inheritance or mixin modules were used. Also, maybe make the association one polymorphic has many instead of multiple 1:1's (but still, each user should only have one of each type of story).

Any task that depends on db:drop (e.g. db:reset and db:populate) will drop the seed_migrations table. When a new seed migration is run after this, it then thinks that no seed migrations have been run yet, so it runs them all and data is duplicated.

Two possible solutions:
(a) Add pre-requisite rake task to db:drop that will write the seed_migrations table to a YAML file. Have another task that will load the seed_migrations table back into memory and is a post-requisite for db:schema:load.
(b) Add a way to record the current version somewhere, and record it after each migration is run. We obviously wouldn't want to record the current version in the database, because we'd run into the same problem... so maybe in a file somewhere? Once we have this recorded somewhere, we could either use it to regenerate the seed_migrations table, or we could use the seed_migrations table in combination with the recorded current version when determining which migrations to run.

For example, we include ActiveRecordSupplement directly into ActiveRecord::Base. With Rails 5.0, we can include this in ApplicationRecord instead.

It looks really ugly now... I think this started after I added min_width: 0 in order to allow the flex box to decrease in size

In sections/_show.html.haml, we can switch params: { 'climb_log[climb_id]': climb.id } back to a proper nested hash once we upgrade to Rails 5.0.

Does this happen for updating as well?

Do this once we have another place (aside from climb type) where we want a radio button group.

So far, this is just for when the new section fields are loading (which probably shouldn't be through AJAX...). It will also be on the gym show page when a new section is selected.

Maybe see if there is a way to profile it?

For example, try logging climbs and then closing the flash message.

Some could probably be cleaned up with page objects. Also, look at other people's feature specs for ideas.

What happens? It should just assign the role to the user.

Change invalid: "Invalid email or password." back to "Invalid %{authentication_keys} or password." once heartcombo/devise#4095 is addressed

Then we can add validation that ensures that when users are given a new role, that it is a role that exists. Then we don't need to validation like this:

# spec/factories/users.rb
# ...
valid_roles = roles.pluck(:name).keep_if do |role_name|
  Employment.roles.include? role_name.to_sym
end
# ...