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 the workshop edit to use the form partial #1155

Open

Conversation

@jabawack81
Copy link
Contributor

jabawack81 commented Nov 12, 2019

working on this issue I had to disable the Foundation validation on the form because it was preventing the sending of the form so I disabled it and now it works as expected.

In reference to issue #1118

jabawack81 added 3 commits Nov 12, 2019
with the validaiton in place the form was prevented to be sent but no error where displayed to the user
this input was the main difference between this partial and the _edit_form partial
@@ -35,6 +35,8 @@
= f.input :rsvp_open_local_time, as: :string, input_html: { data: { value: @workshop.rsvp_opens_at.try(:time).try(:strftime, '%H:%M') } }
.row
.large-6.columns
%label Organisers
= f.collection_select :organisers, Member.all, :id, :full_name, { selected: @workshop.organisers.map(&:id) }, { multiple: true }

This comment has been minimized.

Copy link
@notapatch

notapatch Nov 13, 2019

Contributor

So, my first PR to the project was accepted in September. I say this because I have no project/business knowledge to fall back on.

We are making a change in the behaviour of the code. Before Create was different from Update and with this commit they are the same. It is unusual that we have differences in forms but it's not unheard of. So, have you understood the reasons @ + despo (an original commiter but no longer active) wrote the behaviour the way they did and allowed for that? I say this because the maintainers are going to ask.

@@ -1,4 +1,4 @@
= simple_form_for [:admin, @workshop], url: :admin_workshops do |f|
= simple_form_for [:admin, @workshop], html: { novalidate: true } do |f|

This comment has been minimized.

Copy link
@notapatch

notapatch Nov 13, 2019

Contributor

I haven't looked at this yet but I did a quick search for novalidate so my first question is going to be "why novalidate isn't used in any other forms?" So I can be reassured that introducing something new is the only option. A consistent project is an "easy" to maintain project and exceptions in the code makes maintenance harder.

@jabawack81

This comment has been minimized.

Copy link
Contributor Author

jabawack81 commented Nov 15, 2019

that is a good point argument but I've looked at the history of the two files and they were created almost identical in the same commit 7ef5049 and they had a fairly parallel edit history so I'm confident there should be no problem with this PR

@notapatch

This comment has been minimized.

Copy link
Contributor

notapatch commented Nov 15, 2019

Right, fairly parrell, except this line so lets make sure. If you change out of the branch to master. And do git blame against that file.

git blame app/views/admin/workshops/_edit_form.html.haml

And roll down to that line and copy the sha and do git show on it. You'll notice that the committer is despo (who made the original commit of the project) had particular requirements when she introduced this code.

default chapter organisers as workshop organisers on creation
when updating enable user to set new/remove organisers

So, if I was a maintainer, I would want to know that the person making this PR had seen that and knew that this code wouldn't change that - this is the only line we can get wrong :-) And because this business knowledge will disappear with the deletion of the file (to all but the most determined reader).

So, and I haven't tried this, but with the change:

  • Does a new workshop still add the chapter organisers automatically?
  • Is this tested so we can't fork up?

Because if we break that we're going to be really unpopular with the most influential people in codebar :) And after a while of hunting you down they'll work out that the [X] isn't actually a mis-loaded image and then you'll be in big trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.