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

[ROAD 534] Add state to stories #309

Merged
merged 22 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
16c3492
Add nokogiri for apple silicon
mateusdeap Sep 26, 2023
45149a9
Add `approved` field to the stories table
mateusdeap Sep 26, 2023
37225f9
Add `pending` method with specs
mateusdeap Sep 28, 2023
a0786cc
Add `update_story` controller action
mateusdeap Sep 28, 2023
974d647
Add frontend
mateusdeap Sep 28, 2023
3d79fda
Use path helper instead of url
mateusdeap Sep 28, 2023
fa11b34
Change button color with approval status
mateusdeap Sep 28, 2023
97e8dbf
Use enums for story status
mateusdeap Oct 2, 2023
87949c2
Use remote: true for status button and refactor routes
mateusdeap Oct 2, 2023
9528442
Add appropriate specs and modify existing ones to the new changes
mateusdeap Oct 2, 2023
342db07
Move story related css to appropriate file
mateusdeap Oct 2, 2023
740ea12
Change back to headless firefox
mateusdeap Oct 4, 2023
48cbbb2
Merge branch 'main' of github.com:fastruby/points into ROAD-534-add-a…
arielj Oct 9, 2023
e10e5ad
Refactor routes
mateusdeap Oct 9, 2023
08d344c
Apply suggestions from code review
mateusdeap Oct 9, 2023
a88dab2
Add status grid-area in scss
mateusdeap Oct 10, 2023
324c2e8
Refactor label update logic
mateusdeap Oct 10, 2023
85d21ed
Move status field to last position
mateusdeap Oct 10, 2023
c934650
Merge branch 'main' into ROAD-534-add-approved-attribute-to-stories
mateusdeap Oct 11, 2023
cf3af58
Remove Rejected as an option on story creation
mateusdeap Oct 11, 2023
a7bfb2c
Add specs to test status selection on story creation
mateusdeap Oct 11, 2023
38f4b44
Replace `row` for query selector
mateusdeap Oct 11, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ GEM
next_rails (1.2.2)
colorize (>= 0.8.1)
nio4r (2.5.8)
nokogiri (1.14.3-arm64-darwin)
racc (~> 1.4)
nokogiri (1.14.3-x86_64-darwin)
racc (~> 1.4)
nokogiri (1.14.3-x86_64-linux)
Expand Down Expand Up @@ -399,6 +401,7 @@ GEM
zeitwerk (2.6.7)

PLATFORMS
arm64-darwin-22
x86_64-darwin-21
x86_64-linux

Expand Down
10 changes: 10 additions & 0 deletions app/assets/javascripts/stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,13 @@ document.addEventListener("DOMContentLoaded", () => {
});
});
});

function updateStatusButton(color, status) {
const button = document.querySelector(".story-title .dropdown-wrapper > button");
button.className = `button ${color}`;

const span = button.querySelector("span");
span.textContent = status;

document.querySelector(":focus").blur();
}
26 changes: 26 additions & 0 deletions app/assets/stylesheets/3-atoms/_badges.scss
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,29 @@
background-color: #57ce81;
}
}

.story-status-badge {
display: inline-block;
padding: 7px 15px;
font-weight: 600;
line-height: 1;
text-align: center;
white-space: nowrap;
vertical-align: baseline;
border-radius: 20px;
font-size: 11px;
background-color: $orange;

&.approved {
background-color: $green;
border-color: #1d4ed8;
color: $white;
}

&.rejected {
background-color: $magenta;
border-color: #d77e72;
color: $white;
}

}
2 changes: 1 addition & 1 deletion app/assets/stylesheets/4-molecules/_tables.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
}
.project-table__row {
display: grid;
grid-template-columns: 1fr 70px 70px 260px;
grid-template-columns: 1fr 100px 70px 70px 260px;
align-items: center;
padding: 10px 0;
&.project-table__row--reports {
Expand Down
6 changes: 6 additions & 0 deletions app/assets/stylesheets/stories.scss
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,9 @@
border: 1px solid #9b054d;
padding: 5px;
}

.story-title {
display: flex;
align-items: center;
gap: 1rem;
}
27 changes: 24 additions & 3 deletions app/controllers/stories_controller.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
require "csv"
class StoriesController < ApplicationController
before_action :authenticate_user!
before_action :find_project, except: [:bulk_destroy, :render_markdown, :edit, :update, :destroy, :show, :move]
before_action :find_story, only: [:edit, :update, :destroy, :show, :move]
before_action :find_project, except: [:bulk_destroy, :render_markdown, :edit, :update, :destroy, :show, :move, :approve, :reject, :pending]
before_action :find_story, only: [:edit, :update, :destroy, :show, :move, :approve, :reject, :pending]
before_action :validate_url_product_id, only: [:edit, :update, :destroy, :show, :move]
before_action :ensure_unarchived!, except: [:show, :bulk_destroy, :render_markdown, :move]

Expand Down Expand Up @@ -112,6 +112,27 @@ def move
redirect_to @project
end

def approve
@story.approved!
respond_to do |format|
format.js { render "shared/update_status" }
end
end

def reject
@story.rejected!
respond_to do |format|
format.js { render "shared/update_status" }
end
end

def pending
@story.pending!
respond_to do |format|
format.js { render "shared/update_status" }
end
end

JuanVqz marked this conversation as resolved.
Show resolved Hide resolved
private

def find_project
Expand All @@ -128,7 +149,7 @@ def validate_url_product_id
end

def stories_params
params.require(:story).permit(:title, :description, :extra_info, :project_id)
params.require(:story).permit(:title, :description, :extra_info, :project_id, :approved)
mateusdeap marked this conversation as resolved.
Show resolved Hide resolved
end

def expected_csv_headers?(file)
Expand Down
11 changes: 11 additions & 0 deletions app/helpers/stories_helper.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,13 @@
module StoriesHelper
def status_label(story)
arielj marked this conversation as resolved.
Show resolved Hide resolved
"<span class='story-status-badge #{story.status}'>#{story.status}</span>".html_safe
Copy link
Member

@JuanVqz JuanVqz Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see the first letter as uppercase equal as we see it in the select field.
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to skip this one since I think labels are better in lowercase but, mainly, because when I started to change this, I noticed I'd have to edit quite a few specs and a bit of JS that was swapping css classes based on the text content of the label.

Maybe add this as a bug or improvement story for later on?

end

def status_color(story)
return "green" if @story.approved?

return "magenta" if @story.rejected?

return "orange" if @story.pending?
end
mateusdeap marked this conversation as resolved.
Show resolved Hide resolved
end
2 changes: 2 additions & 0 deletions app/models/story.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ class Story < ApplicationRecord

before_create :add_position

enum :status, [:pending, :approved, :rejected]

scope :by_position, -> { order("stories.position ASC NULLS FIRST, stories.created_at ASC") }

def best_estimate_average
Expand Down
13 changes: 8 additions & 5 deletions app/views/estimates/_update_row.js.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
let row = document.getElementById("story_<%= estimate.story_id %>")
row.children[1].innerText = "<%= j(estimate.best_case_points.to_s) %>"
row.children[2].innerText = "<%= j(estimate.worst_case_points.to_s) %>"
status_label = row.querySelector(".status > .story-status-badge")
status_label.textContent = "<%= estimate.story.status %>"
status_label.classList.value = "story-status-badge <%= estimate.story.status %>"

let totals_row = document.querySelector('.project-table tfoot tr')
totals_row.children[1].innerText = "<%= j @project.best_estimate_sum_per_user(current_user) %>"
totals_row.children[2].innerText = "<%= j @project.worst_estimate_sum_per_user(current_user) %>"
document.getElementById("best_estimate_<%= estimate.story_id %>").innerText = "<%= j(estimate.best_case_points.to_s) %>"
document.getElementById("worst_estimate_<%= estimate.story_id %>").innerText = "<%= j(estimate.worst_case_points.to_s) %>"

document.querySelector('.project-table tfoot tr > .best_estimates_total').innerText = "<%= j @project.best_estimate_sum_per_user(current_user) %>"
document.querySelector('.project-table tfoot tr > .worst_estimates_total').innerText = "<%= j @project.worst_estimate_sum_per_user(current_user) %>"
2 changes: 1 addition & 1 deletion app/views/estimates/create.js.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@
<% else %>
updateModal("New estimate", "<%= j(render partial: 'modal_body') %>")
<% end %>
})()
})()
13 changes: 8 additions & 5 deletions app/views/projects/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<thead class="table-header fixed-header">
<tr class="project-table__row project-table__row--header">
<th class="project-table__cell">Story Title</th>
<th class="project-table__cell">Status</th>
arielj marked this conversation as resolved.
Show resolved Hide resolved
<th class="project-table__cell">Best<br />Estimate</th>
<th class="project-table__cell">Worst<br />Estimate</th>
<th class="project-table__cell story_actions">
Expand All @@ -25,14 +26,15 @@
<tbody id="stories" data-url="<%= sort_stories_project_path(@project.id) %>" data-unlocked="<%= is_unlocked?(@project) %>">
<% if @stories.present? %>
<% @stories.each do | story | %>
<tr class="project-table__row project-table__row--story" id="<%= dom_id(story)%>" >
<tr class="project-table__row project-table__row--story" id="<%= dom_id(story)%>" data-status="<%= story.status %>">
<td class="project-table__cell">
<input type="checkbox" name="stories[]" value="<%= story.id %>">
<%= link_to story.title, [story.project, story] %>
<button class="copy-link btn-clipboard" data-clipboard-text="<%= project_story_url(@project, story) %>" title='Copy story'><i class="fa fa-link"></i><span class= "popup">Copied to clipboard</span></button>
</td>
<td class="project-table__cell"><%= story.estimate_for(current_user)&.best_case_points %></td>
<td class="project-table__cell"><%= story.estimate_for(current_user)&.worst_case_points %></td>
<td class="project-table__cell status"><%= status_label(story) %></td>
<td class="project-table__cell" id="best_estimate_<%= story.id %>"><%= story.estimate_for(current_user)&.best_case_points %></td>
<td class="project-table__cell" id="worst_estimate_<%= story.id %>"><%= story.estimate_for(current_user)&.worst_case_points %></td>
<td class="project-table__cell story_actions">
<% if is_unlocked?(@project) %>
<% if estimated(story) %>
Expand Down Expand Up @@ -83,8 +85,9 @@
<tfoot>
<tr class="project-table__row project-table__row--footer">
<td class="project-table__cell">Total estimates</td>
<td class="project-table__cell"><%= @project.best_estimate_sum_per_user(current_user) %></td>
<td class="project-table__cell"><%= @project.worst_estimate_sum_per_user(current_user) %></td>
<td class="project-table__cell"></td>
<td class="project-table__cell best_estimates_total"><%= @project.best_estimate_sum_per_user(current_user) %></td>
<td class="project-table__cell worst_estimates_total"><%= @project.worst_estimate_sum_per_user(current_user) %></td>
<td class="project-table__cell"></td>
</tr>
</tfoot>
Expand Down
15 changes: 14 additions & 1 deletion app/views/shared/_story.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,17 @@
<h4 class="word-break">Story #<%= story.id %>: <%= story.title %></h4>
<div class="story-title">
<h4 class="word-break">Story #<%= story.id %>: <%= story.title %></h4>
<div class="dropdown-wrapper">
<button class="button <%= status_color(story) %>">
<span><%= story.status %></span>
<i class="fa fa-solid fa-caret-down"></i>
</button>
<div class="dropdown">
<%= button_to "Approve", approve_story_path(@story), class: "status-selector", method: :patch, remote: true %>
<%= button_to "Reject", reject_story_path(@story), class: "status-selector", method: :patch, remote: true %>
<%= button_to "Pending", pending_story_path(@story), class: "status-selector", method: :patch, remote: true %>
</div>
</div>
</div>

<div class="story-description">
<%= markdown(story.description) %>
Expand Down
1 change: 1 addition & 0 deletions app/views/shared/update_status.js.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
updateStatusButton("<%= status_color(@story) %>", "<%= @story.status %>");
5 changes: 5 additions & 0 deletions app/views/stories/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@

<%= f.hidden_field :project_id, value: params[:project_id] %>

<div class="field story_status">
<%= f.label :status, "Status" %>
<%= f.select :status, options_for_select({ "Pending" => "pending", "Approved" => "approved", "Rejected" => "rejected" }, selected: @story.status), class: "project-story-approved" %>
</div>
Copy link
Member

@JuanVqz JuanVqz Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new status field, this seems a little bit moved, I expected the preview to be aligned with the description field at least as it was before.

The first thing I want to write is the story's title. can we move the status field after the extra info?

All of this is purely personal opinion. so, if somebody else can comment about it would be nice.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Will fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I moved the status selector, but I noticed that the description preview is actually in the right place, according to the scss and the app version currently in production.


<div class="field story_title">
<%= f.label :title %>
<%= f.text_field :title, placeholder: "Story Title", class: "project-story-title", autofocus: true %>
Expand Down
4 changes: 4 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
get "home/index"
get "reports/index"

patch "stories/:id/approve", to: "stories#approve", as: "approve_story"
patch "stories/:id/reject", to: "stories#reject", as: "reject_story"
patch "stories/:id/pending", to: "stories#pending", as: "pending_story"
arielj marked this conversation as resolved.
Show resolved Hide resolved

resource :stories do
post :bulk_destroy, to: "stories#bulk_destroy"
post :render_markdown
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20230829001347_add_status_to_stories.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddStatusToStories < ActiveRecord::Migration[7.0]
def change
add_column :stories, :status, :integer, default: 0
end
end
1 change: 1 addition & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
t.integer "position"
t.integer "real_score"
t.string "extra_info"
t.integer "status", default: 0
end

create_table "users", force: :cascade do |t|
Expand Down
27 changes: 27 additions & 0 deletions spec/controllers/stories_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,5 +144,32 @@
expect(response).to redirect_to project2
end
end

describe "#approve" do
it "updates the story status to approved" do
patch :approve, params: {id: story.id}, format: :js

expect(story.reload.status).to eq("approved")
expect(response).to render_template("shared/update_status")
end
end

describe "#reject" do
it "updates the story status to rejected" do
patch :reject, params: {id: story.id}, format: :js

expect(story.reload.status).to eq("rejected")
expect(response).to render_template("shared/update_status")
end
end

describe "#pending" do
it "updates the story status to pending" do
patch :pending, params: {id: story.id}, format: :js

expect(story.reload.status).to eq("pending")
expect(response).to render_template("shared/update_status")
end
end
end
end
47 changes: 47 additions & 0 deletions spec/features/story_show_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
require "rails_helper"

RSpec.describe "story status", js: true do
let(:user) { FactoryBot.create(:user) }
let(:project) { FactoryBot.create(:project) }
let!(:story) { FactoryBot.create(:story, project: project) }

before do
login_as(user, scope: :user)
end

context "from the show page" do
it "allows me to change the story status" do
visit project_story_path(project_id: project.id, id: story.id)

status_button = find(".dropdown-wrapper").find("button")

status_button.click
click_button("Approve")

status_button = find(".dropdown-wrapper").find("button")

expect(status_button).to have_text("approved")
end
end

context "from the estimation modal" do
it "allows me to change the story status" do
visit project_path(id: project.id)
click_link "Add Estimate"

status_button = find(".story-title").find(".dropdown-wrapper").find("button")

status_button.click
click_button("Approve")

status_button = find(".story-title").find(".dropdown-wrapper").find("button")

expect(status_button).to have_text("approved")

click_button "Save changes"
mateusdeap marked this conversation as resolved.
Show resolved Hide resolved

status_badge = find(".project-table").find(".status").find(".story-status-badge")
expect(status_badge).to have_text("approved")
end
end
end
6 changes: 3 additions & 3 deletions spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
browser_options.args << "-headless"
Capybara::Selenium::Driver.new(app, browser: :firefox, options: browser_options)
end
Capybara.javascript_driver = :headless_firefox
Capybara.javascript_driver = :selenium
arielj marked this conversation as resolved.
Show resolved Hide resolved
Capybara.server = :puma, {Silent: true}

# Requires supporting ruby files with custom matchers and macros, etc, in
Expand Down Expand Up @@ -63,8 +63,8 @@ def expect_closed_modal

def expect_story_estimates(story, best, worst)
within_story_row(story) do
expect(find("td:nth-child(2)")).to have_text best.to_s
expect(find("td:nth-child(3)")).to have_text worst.to_s
expect(find("td:nth-child(3)")).to have_text best.to_s
expect(find("td:nth-child(4)")).to have_text worst.to_s
end
end
end
Expand Down
Loading