Merge pull request #119 from publify/setup-password-validation-feedback

Provide proper validation feedback during setup

app/controllers/articles_controller.rb

@@ -2,8 +2,8 @@
 
 class ArticlesController < ContentController
   before_action :login_required, only: [:preview, :preview_page]
-  before_action :auto_discovery_feed, only: [:show, :index]
   before_action :verify_config
+  before_action :auto_discovery_feed, only: [:show, :index]
 
   layout :theme_layout, except: [:trackback]
 

app/controllers/setup_controller.rb

@@ -4,23 +4,24 @@ class SetupController < BaseController
   before_action :check_config
   layout "accounts"
 
-  def index; end
+  def index
+    this_blog.blog_name = ""
+    @user = User.new
+  end
 
   def create
-    this_blog.blog_name = params[:setting][:blog_name]
+    this_blog.blog_name = blog_params[:blog_name]
     this_blog.base_url = blog_base_url
 
-    @user = User.new(login: "admin",
-                     email: params[:setting][:email],
-                     password: params[:setting][:password],
-                     text_filter_name: this_blog.text_filter,
-                     nickname: "Publify Admin")
+    @user = User.new(user_params.merge(login: "admin",
+                                       text_filter_name: this_blog.text_filter,
+                                       nickname: "Publify Admin"))
     @user.name = @user.login
 
-    unless this_blog.save && @user.save
-      redirect_to setup_url
-      return
-    end
+    return render :index unless this_blog.valid? && @user.valid?
+
+    this_blog.save!
+    @user.save!
 
     sign_in @user
 
@@ -36,6 +37,14 @@ def create
 
   private
 
+  def blog_params
+    params.require(:blog).permit(:blog_name)
+  end
+
+  def user_params
+    params.require(:user).permit(:email, :password)
+  end
+
   def create_first_post(user)
     this_blog.articles.build(title: I18n.t("setup.article.title"),
                              author: user.login,

app/views/setup/index.html.erb

@@ -1,17 +1,48 @@
+<div class="row">
+  <div class="col-md-8 col-md-offset-2" id="error-message-page">
+    <% if this_blog.errors.any? %>
+      <div id="error_explanation">
+        <h2><%= t("errors.template.header", model: 'blog', count: this_blog.errors.count) %></h2>
+        <p><%= t("errors.template.body") %></p>
+        <ul>
+          <% this_blog.errors.full_messages.each do |message| %>
+            <li><%= message %></li>
+          <% end %>
+        </ul>
+      </div>
+    <% end %>
+    <% if @user.errors.any? %>
+      <div id="error_explanation">
+        <h2><%= t("errors.template.header", model: 'blog', count: @user.errors.count) %></h2>
+        <p><%= t("errors.template.body") %></p>
+        <ul>
+          <% @user.errors.full_messages.each do |message| %>
+            <li><%= message %></li>
+          <% end %>
+        </ul>
+      </div>
+    <% end %>
+  </div>
+</div>
+
 <%= form_tag action: 'index' do %>
   <div class='alert alert-info'>
     <small><%= t('.welcome_to_your_blog_setup', publify: link_to('Publify', 'https://publify.github.io/')) %></small>
   </div>
-  <div class='form-group'>
-    <%= text_field(:setting, :blog_name, class: 'form-control', placeholder: t('.blog_name')) %>
-  </div>
-  <div class='form-group'>
-    <%= text_field(:setting, :email, class: 'form-control', placeholder: t('.your_mail')) %>
-  </div>
-  <div class='form-group'>
-    <%= label_tag :setting_password, t('.password') %><br />
-    <%= password_field(:setting, :password, class: 'form-control') %>
-  </div>
+  <%= fields model: this_blog do |form| %>
+    <div class='form-group'>
+      <%= form.text_field(:blog_name, class: 'form-control', placeholder: t('.blog_name')) %>
+    </div>
+  <% end %>
+  <%= fields model: @user do |form| %>
+    <div class='form-group'>
+      <%= form.text_field(:email, class: 'form-control', placeholder: t('.your_mail')) %>
+    </div>
+    <div class='form-group'>
+      <%= form.label :password, t('.password') %><br>
+      <%= form.password_field(:password, class: 'form-control') %>
+    </div>
+  <% end %>
 
-  <input type="submit" id="submit" class='btn btn-lg btn-success btn-block' value="<%= t('generic.save') %>" />
+  <input type="submit" id="submit" class='btn btn-lg btn-success btn-block' value="<%= t('generic.save') %>">
 <% end %>

db/seeds.rb

@@ -13,9 +13,9 @@
 blog = Blog.first || Blog.create!
 
 unless blog.sidebars.any?
-  PageSidebar.create!(active_position: 0, staged_position: 0, blog_id: blog.id)
-  TagSidebar.create!(active_position: 1, blog_id: blog.id)
-  ArchivesSidebar.create!(active_position: 2, blog_id: blog.id)
-  StaticSidebar.create!(active_position: 3, blog_id: blog.id)
-  MetaSidebar.create!(active_position: 4, blog_id: blog.id)
+  PageSidebar.create!(active_position: 0, staged_position: 0, blog: blog)
+  TagSidebar.create!(active_position: 1, blog: blog)
+  ArchivesSidebar.create!(active_position: 2, blog: blog)
+  StaticSidebar.create!(active_position: 3, blog: blog)
+  MetaSidebar.create!(active_position: 4, blog: blog)
 end

spec/controllers/setup_controller_spec.rb

@@ -6,14 +6,20 @@
   let(:strong_password) { "fhnehnhfiiuh" }
 
   describe "#index" do
-    describe "when no blog is configured" do
+    describe "when blog is not configured" do
+      render_views
+
       before do
-        # Set up database similar to result of seeding
-        @blog = Blog.create
+        # Set up database similar to result of db:setup
+        Blog.create
         get "index"
       end
 
       specify { expect(response).to render_template("index") }
+
+      it "does not show the default blog name in the form" do
+        expect(response.body).to have_css "input#blog_blog_name[value='']"
+      end
     end
 
     describe "when a blog is configured and has some users" do
@@ -27,26 +33,28 @@
   end
 
   describe "#create" do
-    context "when no blog is configured" do
-      before do
-        # Set up database similar to result of seeding
-        @blog = Blog.create
-      end
+    context "when blog is not configured" do
+      # Set up database similar to result of seeding
+      let!(:blog) { Blog.create }
 
       context "when passing correct parameters" do
         before do
           ActionMailer::Base.deliveries.clear
-          post :create, params: { setting: { blog_name: "Foo", email: "[email protected]",
-                                             password: strong_password } }
+          post :create, params: { blog: { blog_name: "Foo" },
+                                  user: { email: "[email protected]",
+                                          password: strong_password } }
         end
 
         it "correctly initializes blog and users" do
-          expect(Blog.first.blog_name).to eq("Foo")
           admin = User.find_by(login: "admin")
-          expect(admin).not_to be_nil
-          expect(admin.email).to eq("[email protected]")
-          expect(Article.first.user).to eq(admin)
-          expect(Page.first.user).to eq(admin)
+
+          aggregate_failures do
+            expect(Blog.first.blog_name).to eq("Foo")
+            expect(admin).not_to be_nil
+            expect(admin.email).to eq("[email protected]")
+            expect(Article.first.user).to eq(admin)
+            expect(Page.first.user).to eq(admin)
+          end
         end
 
         it "logs in admin user" do
@@ -63,37 +71,54 @@
         end
       end
 
-      describe "when passing incorrect parameters" do
-        it "empty blog name should raise an error" do
-          post :create, params: { setting: { blog_name: "", email: "[email protected]",
-                                             password: strong_password } }
-          expect(response).to redirect_to(action: "index")
+      context "when passing incorrect parameters" do
+        it "does no setup when blog name is empty" do
+          post :create, params: { blog: { blog_name: "" },
+                                  user: { email: "[email protected]",
+                                          password: strong_password } }
+          aggregate_failures do
+            expect(response).to render_template "index"
+            expect(blog.reload).not_to be_configured
+          end
         end
 
-        it "empty email should raise an error" do
-          post :create, params: { setting: { blog_name: "Foo", email: "",
-                                             password: strong_password } }
-          expect(response).to redirect_to(action: "index")
+        it "does no setup when email is empty" do
+          post :create, params: { blog: { blog_name: "Foo" },
+                                  user: { email: "",
+                                          password: strong_password } }
+          aggregate_failures do
+            expect(response).to render_template "index"
+            expect(blog.reload).not_to be_configured
+          end
         end
 
-        it "empty password should raise an error" do
-          post :create, params: { setting: { blog_name: "Foo", email: "[email protected]",
-                                             password: "" } }
-          expect(response).to redirect_to(action: "index")
+        it "does no setup when password is empty" do
+          post :create, params: { blog: { blog_name: "Foo" },
+                                  user: { email: "[email protected]",
+                                          password: "" } }
+          aggregate_failures do
+            expect(response).to render_template "index"
+            expect(blog.reload).not_to be_configured
+          end
         end
 
-        it "weak password should raise an error" do
-          post :create, params: { setting: { blog_name: "Foo", email: "[email protected]",
-                                             password: "foo123bar" } }
-          expect(response).to redirect_to(action: "index")
+        it "does no setup when password is weak" do
+          post :create, params: { blog: { blog_name: "Foo" },
+                                  user: { email: "[email protected]",
+                                          password: "foo123bar" } }
+          aggregate_failures do
+            expect(response).to render_template "index"
+            expect(blog.reload).not_to be_configured
+          end
         end
       end
     end
 
     describe "when a blog is configured and has some users" do
       before do
         create(:blog)
-        post :create, params: { setting: { blog_name: "Foo", email: "[email protected]" } }
+        post :create, params: { blog: { blog_name: "Foo" },
+                                user: { email: "[email protected]" } }
       end
 
       specify { expect(response).to redirect_to(controller: "articles", action: "index") }

spec/features/setup_spec.rb

@@ -18,9 +18,9 @@
     expect(page).to have_text I18n.t!("setup.index.welcome_to_your_blog_setup")
 
     # Set up the blog
-    fill_in :setting_blog_name, with: "Awesome blog"
-    fill_in :setting_email, with: "[email protected]"
-    fill_in :setting_password, with: strong_password
+    fill_in :blog_blog_name, with: "Awesome blog"
+    fill_in :user_email, with: "[email protected]"
+    fill_in :user_password, with: strong_password
     click_button I18n.t!("generic.save")
 
     # Confirm set up success
@@ -48,4 +48,27 @@
     # Confirm proper setting fo user properties
     expect(User.first.email).to eq "[email protected]"
   end
+
+  scenario "setup fails at first due to password weakness" do
+    visit "/setup"
+    fill_in :blog_blog_name, with: "Awesome blog"
+    fill_in :user_email, with: "[email protected]"
+    fill_in :user_password, with: "not-strong"
+    click_button I18n.t!("generic.save")
+
+    expect(page)
+      .to have_text "Password not strong enough. It scored 2. It must score at least 4."
+
+    fill_in :user_password, with: strong_password
+    click_button I18n.t!("generic.save")
+
+    expect(page).to have_text I18n.t!("accounts.confirm.success")
+  end
+
+  scenario "setup fails due to missing blog name" do
+    visit "/setup"
+    click_button I18n.t!("generic.save")
+
+    expect(page).to have_text "Blog name can't be blank"
+  end
 end