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

Refactor error messages in Ruby on Rails forms to get rid of "dynamic_form" gem #8545

Closed
3 tasks
jywarren opened this issue Oct 12, 2020 · 14 comments · Fixed by #8550 or #8553
Closed
3 tasks

Refactor error messages in Ruby on Rails forms to get rid of "dynamic_form" gem #8545

jywarren opened this issue Oct 12, 2020 · 14 comments · Fixed by #8550 or #8553
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration bug the issue is regarding one of our programs which faces problems when a certain task is executed fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet refactorization

Comments

@jywarren
Copy link
Member

jywarren commented Oct 12, 2020

We use the dynamic_form gem in many places for reasons that we can't even remember -- so if we try to remove that gem, as we did in #4575, tests fail. So let's fix this all and then circle back to that pull request to finish up!

Let's go through the uses of this gem everywhere we can find it and replace it with a more modern and standard form syntax! They look like this, using the error_messages property of the form_for block:

<%= form_for :revision, :as => :revision, :url => url, :html => {:class => "form"} do \|f\| %>
  <% if f.error_messages != "" %>
    <div class="alert alert-danger"><%= f.error_messages :header_message => "Your note couldn't be saved." %></div>
  <% end %>
<% end %>

But they should look like this:

  <% if @model.errors.any? %>
    <div id="error_explanation">
      <h2><%= pluralize(@model.errors.count, "error") %> prohibited this model from being saved:</h2>
      <ul>
      <% @model.errors.full_messages.each do |msg| %>
        <li><%= msg %></li>
      <% end %>
      </ul>
    </div>
  <% end %>

We can search to find where changes should be made, and the simpler of these may potentially be good first-timers-only issues, although if updating these cause tests to need updating too, that could be a potential barrier for a newcomer, so let's be sure to support people well in these!

Here are many instances that could be broken out into their own issues:

https://github.com/publiclab/plots2/search?q=error_messages&unscoped_q=error_messages

(we can keep adding links to instances in this list, as they are completed and checked off!)

@jywarren jywarren added bug the issue is regarding one of our programs which faces problems when a certain task is executed break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet refactorization labels Oct 12, 2020
@jywarren jywarren mentioned this issue Oct 12, 2020
5 tasks
jywarren added a commit that referenced this issue Oct 12, 2020
@jywarren
Copy link
Member Author

I've made a first-timers-only issue from one of these! See how it looks to perform one of these updates by seeing this commit:

eada0c0

@stacytonui
Copy link
Contributor

Hey can I try working on this?

@jywarren
Copy link
Member Author

Hi @stacytonui -- sure, can you choose one from the checklist above, and open a pull request with that one file change, similar to eada0c0 ? And link to it from here? Thank you so much!

@stacytonui
Copy link
Contributor

Okay let me work on the first one

@stacytonui
Copy link
Contributor

@jywarren apologies for the first PR, my repo wasn't synced with the main one

@noi5e
Copy link
Contributor

noi5e commented Oct 12, 2020

@jywarren I'd like to work on the second one in plots2/app/views/users/_edit_form.html.erb if you can reserve it for me :) Will make a PR.

@diorshelton
Copy link

diorshelton commented Oct 12, 2020

Can I give this a shot with plots2/app/views/map/edit.html.erb ?

@diorshelton
Copy link

diorshelton#1 (comment)

@jywarren
Copy link
Member Author

Hi, @diorshelton - I'd love your help - but unfortunately the map/edit.html.erb file is the one I opened up a "first-timers-only" issue for and reserved for @Asu1996 here - #8546

Would you be willing to take on a different one from the list, or from the search results above? Your code looks great! Thank you for your understanding!

@noi5e I'll check out yours now, thank you too!

@jywarren
Copy link
Member Author

Hi @noi5e - are you working on this in #8553 ? Need any help with the errors you're seeing? No worries, I'm here to support!

@Cadreia
Copy link
Contributor

Cadreia commented Oct 13, 2020

Hi, @jywarren can I work on question.html.erb?

@noi5e
Copy link
Contributor

noi5e commented Oct 13, 2020

@jywarren Yes, will reply to you in the PR, thank you!

@jywarren
Copy link
Member Author

Hi, @jywarren can I work on question.html.erb?

Yes, that would be great!!!

jywarren pushed a commit that referenced this issue Oct 13, 2020
@noi5e
Copy link
Contributor

noi5e commented Oct 13, 2020

@jywarren - Looks like there are two instances in post.html.erb:

<% if f.error_messages != "" %><div class="alert alert-danger"><%= f.error_messages :header_message => "Your note couldn't be saved." %></div><% end %>

<% if f.error_messages != "" %><div class="alert alert-danger"><%= f.error_messages :header_message => "Your note couldn't be saved." %></div><% end %>

Happy to give these a shot if that's okay!

piyushswain pushed a commit to piyushswain/plots2 that referenced this issue Oct 22, 2020
manchere pushed a commit to manchere/plots2 that referenced this issue Feb 13, 2021
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this issue Mar 2, 2021
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this issue Oct 16, 2021
ampwang pushed a commit to ampwang/plots2 that referenced this issue Oct 26, 2021
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this issue Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration bug the issue is regarding one of our programs which faces problems when a certain task is executed fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet refactorization
Projects
None yet
5 participants