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

User edit proposals #414

Closed
wants to merge 20 commits into from
Closed

User edit proposals #414

wants to merge 20 commits into from

Conversation

mi-wood
Copy link
Member

@mi-wood mi-wood commented Jan 7, 2018

I've got to step back for a bit and might need a second set of eyes, but here's what I have so far for restroom edit proposals.

The general plan is:

  1. allow users to clock "propose restroom edit" on a restroom page
  2. autofill the form with the current restroom data
  3. submit new restroom with a new edit and an "edit_id" which is the original listing's id. the user then will be redirected to the original restroom page and be given a message that we're taking a look at the edit
  4. allow an admin to view these edits and approve them. once approved, it will update the original listing and delete the edit

The default edit_id is 0, and restroom views will filter for this to avoid showing edits.

So far I've implemented 1,2 and the restroom filter.

I'm having an issue with the edit submission itself, where the form is redirecting to update instead of create. I think this has something to do with the coffeescript/js for submitting a restroom. Essentially, it will run the before_action for :find_restroom and fail. The id it fails on is the id of the edit where I would like the run the create method and redirect to the id of the original (also the edit_id on the copy).

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 8, 2018

[Edit: Reading your original comment above, I can see you are probably aware of everything I wrote in this comment. Sorry for not reading earlier.]

I'm going to start by trying to test the user-facing parts of this from an end-user perspective, since "front-end" is more of my strong area at the moment.

I did get an error when clicking the "submit" button after editing a restroom entry.

Here is a zipped .html file of the error page I see. (I got this by right clicking the error page and doing "Save As..."): Action Controller: Exception caught.html.zip

The error seems to be because the new page is submitted with a restroom entry "id" that isn't already in the database, which, taking a guess, I might expect would happen for adding a totally new restroom, vs submitting an edit.

Seems like the submit button functionality needs to be updated a bit.

By the way this would be a super great feature, users have definitely been asking for it, so I think it's very exciting to see some headway being made on this issue!

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 8, 2018

As for the Travis log:

  1. This test: Scenario: Request edit from restroom # features/contact.feature:3
    is failing, because this PR removes the "contact us about this post!" link in favor of a "Propose an edit to this listing." link. That test needs a quick update to reflect this PR.

  2. This test: Scenario: View an edit # features/edit.feature:7
    is undefined to some extent. (status is technically "undefined," not "failing")...

The step to visit the "edit page" for Winnipeg Restroom is undefined:

      Undefined step: "I visit the edit page for 'Winnepeg Restroom'" (Cucumber::Undefined)
      features/edit.feature:8:in `When I visit the edit page for 'Winnepeg Restroom''`

We need to update the definitions for the test so Cucumber knows how to visit Winnipeg Restroom's edit page.

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.
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 16, 2018

#423 is @stardust66's go at part 3 outlined in the original post, if I understand correctly.

@mi-wood
Copy link
Member Author

mi-wood commented Jan 18, 2018

I removed one of the old "contact" tests. Kept the second to test that past behavior remains the same.

I think the second one was just a typo for a capitol R in the step.

@mi-wood
Copy link
Member Author

mi-wood commented Jan 18, 2018

Looks like last time I might have just not finished the tests I was writing

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 18, 2018

I looked over the step definitions in the cucumber tests, and they seem fine on first glance. I suspect there is a technically relevant typo in there somewhere;

Somehow, an error message is being piped into the logic of the test.

  Scenario: View an edit                               # features/edit.feature:7
    When I visit the edit page for 'Winnepeg Restroom' # features/step_definitions/edit_steps.rb:10
    Then I should see the restroom address             # features/step_definitions/edit_steps.rb:15
      expected to find text "684 East hastings" in "ActiveRecord::RecordNotFound at /restrooms/new ============================================== > Couldn't find Restroom with 'id'= app/controllers/restrooms_controller.rb, line 84 ------------------------------------------------ ``` ruby 79 flash[:alert] = I18n.t('restroom.flash.unexpected') 80 end 81 end 82 83 def find_restroom > 84 @restroom = Restroom.find(params[:id]) 85 end 86 87 def permitted_params 88 params.require(:restroom).permit( 89 :edit_id, ``` App backtrace ------------- - app/controllers/restrooms_controller.rb:84:in `find_restroom' - app/controllers/restrooms_controller.rb:17:in `new' - features/step_definitions/edit_steps.rb:12:in `block in ' Full backtrace -------------- - activerecord (5.1.4) lib/active_record/core.rb:189:in `find' - app/controllers/restrooms_controller.rb:84:in `find_restroom' - app/controllers/restrooms_controller.rb:17:in `new' - actionpack (5.1.4) lib/action_controller/metal/basic_implicit_render.rb:4:in `send_action' - actionpack (5.1.4) lib/abstract_controller/base.rb:186:in `process_action' - actionpack (5.1.4) lib/action_controller/metal/rendering.rb:30:in `process_action'

see Line 1190 of the Travis test log.

(By the way, wading through errors describes most of the time I spent so far on #421, so I know what this sort of thing can be like.)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 18, 2018

Unfortunately, same error as above (edit.feature:7), but also:

Scenario: Submit an edit                             # features/edit.feature:11
    When I submit an edit to 'Winnepeg Restroom'       # features/step_definitions/edit_steps.rb:19
      Unable to find visible field "restroom[name]" that is not disabled (Capybara::ElementNotFound)
      ./features/step_definitions/edit_steps.rb:22:in `/^I submit an edit to 'Winnepeg Restroom'$/'
      features/edit.feature:12:in `When I submit an edit to 'Winnepeg Restroom''
    Then I should see that the edit has been submitted # features/step_definitions/edit_steps.rb:26

@mi-wood
Copy link
Member Author

mi-wood commented Mar 7, 2018

Ok! I've actually got something here now :)

The new process is:

  1. Add a column for edit_id and approved. Only listings with approved = true will show up, and the results will be grouped by edit_id using the most recent id
  2. Allow users to propose edits that will add a new row with edit_id equal to the original listing id
  3. The admin just has to set approved = true for approved edits in active admin.

@tkwidmer would appreciate your eyes on this one!

Anyone else can also pull down the branch and mess around with it. I added some tests and also tested that the geocoding updates for a change in address.

I still need to write a migration to add an edit_id = id for all the existing restrooms.

@mi-wood
Copy link
Member Author

mi-wood commented Jul 28, 2018

Closing in favor of: #487

@mi-wood mi-wood closed this Jul 28, 2018
@mi-wood mi-wood deleted the user-edits branch November 18, 2018 18:23
@DeeDeeG DeeDeeG mentioned this pull request Nov 18, 2018
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

Successfully merging this pull request may close these issues.

3 participants