-
Notifications
You must be signed in to change notification settings - Fork 260
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
User edit proposals - rebased on develop #487
User edit proposals - rebased on develop #487
Conversation
This looks really cool! I wouldn't know how to test that this works. But this has been a big user-requested feature for some time, so it would be pretty awesome to be able to do this. It certainly looks functional in your screenshots, too. 👍 |
I'd like to try to push this through soon, since we're so behind on edits. Looks like code climate is yelling, but other than that, this seems good to me. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This forces the form to submit to /restrooms with POST. Normally, simple_form_for would submit to /restrooms/:id with PATCH if the restroom already exists, which results in invoking the update method on restroom controller.
Add edit_id to permitted params and add it as a hidden field in the form, which allows it to be submitted. The edit_id set in the 'new' method only affects the value of the field in the form, and not the edit_id for the @Restroom in the create method, since they refer to different objects.
Redirect to the original restroom that was edited. params[:id] is nil because the params here are the ones submitted with the form, and there isn't an id field in the form.
It was removed in 9b1437a ("update current scope") but I think that was an accident. We still need the script.
With migration.
Okay, looks like this has been rebased now. Thanks @stardust66. CodeClimate has analyzed this, and says the So, looking into this, we can either try to refactor / break this up and somehow make it simpler, or rather decide it's okay as-is. I don't read/edit the ruby code very much, so other folks' input would be appreciated on this one. Otherwise, there are no remaining issues with CodeClimate or Travis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, this is ready to go.
The Code Climate warning seems pretty minor to me.
This comment has been minimized.
This comment has been minimized.
db/schema.rb
Outdated
@@ -10,7 +10,7 @@ | |||
# | |||
# It's strongly recommended that you check this file into your version control system. | |||
|
|||
ActiveRecord::Schema.define(version: 20180613231032) do | |||
ActiveRecord::Schema.define(version: 2015_10_18_191859) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis apparently reads this older date and thinks we have "pending migrations" and fails.
I recommend just using the same date (2018 06 13, 23:10:32) as before, with or without the underscores. (I don't get what this all means, to tell the truth, but I think it would make Travis pass the tests again.)
Context
Summary of Changes
From #414:
This migration is fine if no edits were added prior to running it (and rails shouldn't let that happen). Someone more familiar with migrations should take a look though.
Checklist
Screenshots of Editing Process