Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

labels not showing for preptimes or feeds on form page #30

Closed
jatwell93 opened this issue May 31, 2017 · 23 comments
Closed

labels not showing for preptimes or feeds on form page #30

jatwell93 opened this issue May 31, 2017 · 23 comments

Comments

@jatwell93
Copy link
Owner

Minor issue but for some reason the labels for preptime and feeds dont show on the form page. Everywhere else they seem to appear.

The code for styles works, which i thought was the exact same.

@Mirv
Copy link
Collaborator

Mirv commented May 31, 2017

Start tossing down the controller#action & then the file you're looking at to help me with locating the issue ... a quick search makes me believe I should be trouble shooting the workouts?

@jatwell93
Copy link
Owner Author

sorry controller: recipes action: new and edit.

@Mirv
Copy link
Collaborator

Mirv commented Jun 2, 2017

Ok, so the behavior on these 3 (style/feeds/preptime) is the same - the issue is programmatically we haven't told the page to serve the labels even if blank. I went & clicked the check boxes next to two I made & then "viola!", we have "Feeds: 1".

From programming & UI - you absolutely don't want the check boxes - the application should worry about whether or not to serve information, not the user. That being said, if you want the user to have the option to hide things, you could simply have Rails set a default of visible & that way later on users can go uncheck the box if they want - which would also save us the most programming time as we don't have to rewrite anything.

Oddly though, right now your code is setup in such a way that it hyperlinks the "1" ... I left the make "feed" & preptime parts alone. I don't see a reason why "feeds" should be a hyper link - unless you want people to be able click that & then see all recipes which feed the same number of people?

@jatwell93
Copy link
Owner Author

jatwell93 commented Jun 3, 2017

Ahhhh that makes a lot of since because i went thought everything and was like "these are the exact same >.<"

Yeh i dont like the check boxes anymore either. When you say the application should serve the information does that mean setting up some way of categorising recipes? Because my initial plan was to add feeds, styles, preptimes and calories (calories feel apart) so the user could add extra information about their recipe before posting it. the i had a simple search where people could find recipes that also had i.e; feeds 2 etc.

@Mirv
Copy link
Collaborator

Mirv commented Jun 3, 2017

Roger, there's some different things in there - so lets break it down then ....

For search purposes you should rely on the databases. 40 years of testing relationships/associations & file methods has shown us that a database executing it's proprietary code will always be 10-10,000 times faster searching for things & that's why there's a .where method in activerecord for rails etc.

I don't do the .where query very much - but some quick searches will pull up examples of people on stackoverflow saying, "I want to search on these 'fields'/'columns' from a table for specific things".

With that in mind lets talk about laying out the database ...

There's some 'forms' databases that are relational should follow to enable the fastest performance & easiest maintenance. The layman's of what I want to focus on is that for the most part, we want all the data related to a "thing" or a category to be in the same place. If it's not on the same table, we need an association to the table that holds all information for that category.

I'm thinking that we need to have the recipe model (table) hold the categories...at least for feeds, preptime, ...for style, as you might want to enumerate I could maybe see another model to avoid like 13 spellings of sechwan or someone using chinese symbols...but that could also easily be controlled by having the form value validated in a function stored in the model...ie:

(Note, I'm sure there's a rails way to loop through an enumerated list like this...but perhaps it's best to store the values in their own table and load them into array using a for loop to plop them into the validation test)

style_check
  case: style.downcase 
  when "chinese"
    save_me = "chinese"
  when "vegitarian"
    save_me
....
  else
    errors.add(:style, “Must be one of ... #{list_of_styles}”)
  end

@Mirv
Copy link
Collaborator

Mirv commented Jun 3, 2017

PS: I just pulled to add in these 3 to the recipes (for now not going to add validation - waiting on some feed back from a friend & going easiest route)

@Mirv
Copy link
Collaborator

Mirv commented Jun 3, 2017

addCategoriesToRecipes is the branch ...

Also, going to load in the changes in Issue #35

@Mirv
Copy link
Collaborator

Mirv commented Jun 3, 2017

Change list

  • install guard gem, once all 3 steps are done, we can activate it via bundle exec guard, so that saving a test file will cause it to run that file's tests automatically

  • install the minitest-guard gem

  • run bundle exec guard init to setup file

  • setup guardfile to watch the right directories (guard-minitest handles most of this,but I have to add the whole directory path for all tests to be watched & executed individually)

  • get devise integrated to the test suite via the inheritance in the test_helper.rb (rails 5 treats all controller tests as integration - but rails 4 doesn't so I've got two inheritances in there)

  • ensure test_helper.rb is required

  • ensure devise is logging us in via the helper for tests sign_in @user or sign_in @user, scope: :admin

  • test for get '/recipes' successful load

  • test for prep_time being filled in

  • test that an edit of prep_time results in a change

  • generate migration for prep_time

@Mirv
Copy link
Collaborator

Mirv commented Jun 5, 2017

  • install guard for auto testing
  • install guard-minitest for the default rails generation of the guard init
  • run bundle exec guard init
  • tweaked guardfile to cover some etc directories
  • generated default tests for recipe model and controller
  • ensured valid test for all 3 fields in place
  • invalid test for description, then summary, then name - already had
  • valid test for prep_times & servings_made along with original 3 above this step
  • added :prep_times, to strong_params in recipes controller (leaving your nested mentions of them for now)
  • added servings_made to strong parmas
  • adding servings_made as category to avoid issues with current feeds implementation
  • removed 'calories_ids' from strong_parameters as you said we were done with that - but it could easily be a field that we allowed to be blank on recipes model
  • formated recipe_params a tiny bit - should not be that long, hard to read

@Mirv
Copy link
Collaborator

Mirv commented Jun 5, 2017

Note to self: Having issues with conflicts in feeds currently, can't assign it a default value as there's some association pulling on it - will go after this one next

@jatwell93
Copy link
Owner Author

Thanks so much for spending the time writing that, i never knew it was such a massive performance difference between the two!

I've been searching the internet for ages now trying to come up with some sort of way to stop multiple spellings for the same ingredient etc. I never thought of the problem like that! it makes a lot of sense to let the database do the work.

Seems like you're making awsome progress btw! :)

@Mirv
Copy link
Collaborator

Mirv commented Jun 6, 2017

  • install guard for auto testing
  • install guard-minitest for the default rails generation of the guard init
  • run bundle exec guard init
  • tweaked guardfile to cover some etc directories
  • generated default tests for recipe model and controller
  • ensured valid test for all 3 fields in place
  • invalid test for description, then summary, then name - already had
  • valid test for prep_times & servings_made along with original 3 above this step
  • added :prep_times, to strong_params in recipes controller (leaving your nested mentions of them for now)

  • added servings_made to strong parmas
  • adding servings_made as category to avoid issues with current feeds implementation
  • removed 'calories_ids' from strong_parameters as you said we were done with that - but it could easily be a field that we allowed to be blank on recipes model
  • formated recipe_params a tiny bit - should not be that long, hard to read

test/models/recipe_test.rb 6/6 passing
test/controllers/recipe_controller_test.rb 0/4 passing

@Mirv
Copy link
Collaborator

Mirv commented Jun 9, 2017

Something is still tripping me up:


Finished in 0.215615s, 4.6379 runs/s, 0.0000 assertions/s.

  1) Error:
RecipesControllerTest#test_should_create_recipe:
ActionController::ParameterMissing: param is missing or the value is empty: recipe
    app/controllers/recipes_controller.rb:97:in `recipe_params'
    app/controllers/recipes_controller.rb:33:in `create'
    test/controllers/recipes_controller_test.rb:36:in `block (2 levels) in <class:RecipesControllerTest>'
    test/controllers/recipes_controller_test.rb:24:in `block in <class:RecipesControllerTest>'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

I checked 30+ stackoverflows yesterday...8 hours solid pounding this one ... best options today...

  • .fetch to make defaults - Link
  • try a merge operation - Link
  • do an end around the strong params - Link
  • rails conventions not followed - I doubt it's this but someone points out capitalization & use of _ are important as rails derives classnames for objects to make routing & params work - Link
  • I'm not sure if this is true or not...but apparently it affects tests when no view exists matching params (this is not my experience, but w/e - desparate here) - Link
  • Found a gist with testing & params for Rails 4 - Link
  • Obviously the hash I'm feeding could be wrong, but no idea where...

Last attempt - replace .require with .fetch

@Mirv
Copy link
Collaborator

Mirv commented Jun 9, 2017

Something is still tripping me up:


Finished in 0.215615s, 4.6379 runs/s, 0.0000 assertions/s.

  1) Error:
RecipesControllerTest#test_should_create_recipe:
ActionController::ParameterMissing: param is missing or the value is empty: recipe
    app/controllers/recipes_controller.rb:97:in `recipe_params'
    app/controllers/recipes_controller.rb:33:in `create'
    test/controllers/recipes_controller_test.rb:36:in `block (2 levels) in <class:RecipesControllerTest>'
    test/controllers/recipes_controller_test.rb:24:in `block in <class:RecipesControllerTest>'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

More guesswork ...

What if the issue is set_user in recipe controller - there's no current_user in tests.

If so .... I'm thinking if I use a hidden field to hold a helper that wraps devise's current_user then the testing might be easier?


Some confirmed - I use the same code format in my app & it works. Why different? I'm not sure. There's tons going on.


Next steps

  • build a skeleton without bells and whistles of the Recipe controller.
  • ensure tests run

Plan Zero:

  • probably replace current recipe controller/views/model
  • add in each new feature back too it

Plan A:

  • Copy tests from working skeleton to recipe controller tests
  • Mirror over changes to the core recipe actions one by one
  • fix view errors if anyone (LOL! there will be many)

@jatwell93
Copy link
Owner Author

Oh my god, you are an absolute machine!

I think we've found another great example of why testing your code is so important. Although makes me worry what we might find down the track....oh well that's future us's problem hahaha.

My initial thoughts are pretty much the same as yours; I think it might be easier to work on a skeleton. There's so much code in there from so many iterations, its kind of like finding a needle in a haystack.

Did the .fetch change make any difference by the way?

@Mirv
Copy link
Collaborator

Mirv commented Jun 10, 2017

.require means it has to be present, .fetch is ... idk, never worked with it - one guy presented it as making your own defaults & another said just a nice work around on the required one. From what I can glean & this is speculation - .require is for variables that everything else depends on and will fail programmatically (aka crash the app/site/etc) & anything not related to nested params (strong params in rails exists to protect against hackers coming in via mass assignment bugs) etc should be checked in the Rails model with validation checks.

@Mirv
Copy link
Collaborator

Mirv commented Jun 10, 2017

I'm probably going to kill 4 hours on my berkley class now. Will check back later.

@Mirv
Copy link
Collaborator

Mirv commented Jun 15, 2017

I took a 3 hour wack at it....found out way more cool stuff....but no fix quite yet...

Interesting things...

  • before_actions do trigger for tests (ie: devise or if you want to write your own check for parameters which I did) - ala - Link
  • While using tests, you can puts YAML::dump(params) to show the parameters
  • What i know for sure one of these is not setting... I made my own custom check via the above link ...
  def check_params
    required = [
      :id, 
      :user_id, 
      :name,
      :summary, 
      :description, 
      :prep_times, 
      :servings_made
      ]
    if required.all? {|k, m| params.has_key? k}
      # here you know params has all the keys defined in required array
      puts "\n\n\All params set\n\n"
    else
      puts "\n\n\One of params not set\n\n"
    end 

I am seeing this message 4 out of 6 times, but I need to rework this script somehow to record which ones pass.


Error from param.inspection:

{"params"=>{"recipe"=>{"servings_made"=>"10", "id"=>"1", "name"=>"This is a name", "summary"=>"MyText2", "description"=>"MyText", "prep_times"=>"0", "user_id"=>"1"}}, "controller"=>"recipes", "action"=>"update", "id"=>"980190962"}

@Mirv
Copy link
Collaborator

Mirv commented Jun 16, 2017

I clunked out there - the last hitch was rails 5 code in the assert params....for comparision...

    # I'm rails 5 -- I don't work in a Rails 4 app, because ...
    # ...the format was changed to be more uniform with json 
    patch recipe_url(@recipe), params: { recipe: { name: "This is a name" } }

    # I'm rails 4 -- I work!
    patch recipe_url(@recipe), recipe: { name: "This is a name" }

@Mirv
Copy link
Collaborator

Mirv commented Jun 16, 2017

Added bonus .... these are some of the weird ways I came up of checking for the issues ... mostly adapted from stackoverflow ...

Note, the check_params & it's alternative work best if before_action :check_param is called as it then slides right into the rails test before it finishes & gives best results.

Also, my controller tests are all integration tests (check their inheritance at top)...as in rails 5 that's how it's done & I don't know what people in rails 4 do...

   def check_params
     required = [
       :id, 
       :user_id, 
       :name,
       :summary, 
       :description, 
       :prep_times, 
       :servings_made
       ]
     if required.all? {|k, m| params.has_key? k}
       # here you know params has all the keys defined in required array
       puts "\n\n\All params set\n\n"
     else
       puts "\n\n\One of params not set\n\n"
     puts YAML::dump(params)
     end  
  end
     def check_params2
       required = [
         :id, 
         :user_id, 
         :name,
         :summary, 
         :description, 
         :prep_times, 
         :servings_made
         ]
         visible_required = []
     puts "----Doing required.each----"
       required.each { |k|  if (params.has_key? k) then visible_required << k else puts "was wrong ... #{k}" end }
     puts "----Doing Passed----" 
      visible_required.each { |k| puts "This is .... #{k}" }
    puts "----Doing params.each----"
      params.each { |k, m| puts "Key: #{k} -- value: #{m}" }
     puts "\n\n Recipe debug: #{YAML::dump(params)} \n\n"  
    puts params.inspect
     end

@jatwell93 check_params is setup with one var per line so I could easily comment it out (the comma's on the end can just stay, they don't hurt) & I kept the word order of the vars in the model.rb's validations &relationships/yaml/test/controller etc the same, generally I try to use the order you find in schema.rb which makes your life WAY easier.

@Mirv
Copy link
Collaborator

Mirv commented Jun 16, 2017

Pull request #41 is ready - I don't know if there will actually be issues merging this - as its been several weeks & if you were in the recipe controller - we are bound to have some conflicts.

I didn't merge as I wanted to make sure you're going over it.

Also, almost none of this actually affects that much of the views layouts - they will need to be altered or cleaned up eventually.

This was referenced Jun 16, 2017
@Mirv Mirv closed this as completed Jun 16, 2017
@jatwell93
Copy link
Owner Author

I love reading through your examples, you make stuff a lot easier to understand than when I read through the stack overflow posts myself.

Also, there was not merge conflicts :) so the branch has successfully been merged.

@Mirv
Copy link
Collaborator

Mirv commented Jun 17, 2017

thanks....we're just lucky people answer SO at all for questions...getting them to format it readable & explain why is a rare thing, much less strip out the useless parts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants