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

Conversation

mateusdeap
Copy link
Member

@mateusdeap mateusdeap commented Sep 28, 2023

Jira Ticket

ROAD-534

Motivation / Context

Fixes #196

This PR adds a new attribute to the story model, called approved, which is a boolean and depending on wether it is true, false or nil it represents 3 possible states to a story: Approved, Rejected or Pending.

By default, all stories are created in the Pending state.

It is possible to define this state during story creation and edition.

Additionally, in the story show page, one can use a dropdown to change the state.

Finally, added labels in the project show page so we know at a glance which stories have been approved or not.

QA / Testing Instructions

Run the application via rails s or docker and visit any project page containing stories in the stories list. You should see they are all pending.

To change the state, click on any story and then click on the dropdown button near the story title and select one of the states in the dropdown.

Additionally, you can click to edit the story and select the state from a select box or, in the project's show page, if you click to add a new story, you should be able to select it's state via a select box as well.

Screenshots:

image
image
image
image
image
image


I will abide by the code of conduct.

db/migrate/20230829001347_add_approved_to_stories.rb Outdated Show resolved Hide resolved
app/helpers/stories_helper.rb Outdated Show resolved Hide resolved
app/helpers/stories_helper.rb Show resolved Hide resolved
app/views/projects/show.html.erb Show resolved Hide resolved
app/views/projects/show.html.erb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
app/assets/stylesheets/project.scss Outdated Show resolved Hide resolved
@mateusdeap mateusdeap requested a review from arielj October 2, 2023 19:59
@mateusdeap
Copy link
Member Author

@arielj Addressed your comments

spec/rails_helper.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@arielj arielj left a comment

Choose a reason for hiding this comment

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

I added one suggestion and I understand the CI tests are only failing for a configuration issue.

I'm approving this so it's ready for QA apart from those 2 comments

config/routes.rb Outdated Show resolved Hide resolved
@arielj
Copy link
Contributor

arielj commented Oct 9, 2023

After merging, we have to run rails db:migrate in heroku's console.

@aisayo aisayo temporarily deployed to points-road-534-add-app-roqgp2 October 9, 2023 19:55 Inactive
Copy link
Member

@JuanVqz JuanVqz left a comment

Choose a reason for hiding this comment

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

Let me know what do you think

app/helpers/stories_helper.rb Outdated Show resolved Hide resolved
<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.

spec/features/story_show_spec.rb Show resolved Hide resolved
app/controllers/stories_controller.rb Show resolved Hide resolved
app/controllers/stories_controller.rb Outdated Show resolved Hide resolved
@mateusdeap mateusdeap temporarily deployed to points-road-534-add-app-roqgp2 October 9, 2023 23:59 Inactive
@mateusdeap mateusdeap temporarily deployed to points-road-534-add-app-roqgp2 October 10, 2023 19:36 Inactive
@mateusdeap mateusdeap temporarily deployed to points-road-534-add-app-roqgp2 October 11, 2023 10:56 Inactive
Copy link
Member

@JuanVqz JuanVqz left a comment

Choose a reason for hiding this comment

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

I left a comment but I'll approve it since I've been so annoying. @mateusdeap Thanks!

@@ -1,2 +1,18 @@
module StoriesHelper
def status_label(story)
"<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?

@FionaDL
Copy link
Member

FionaDL commented Oct 12, 2023

@mateusdeap feel free to merge after addressing Juan's comment (if you feel it's worth it), but don't forget to run the migrations in heroku console :)

@mateusdeap mateusdeap merged commit 76d3e74 into main Oct 12, 2023
2 checks passed
@mateusdeap mateusdeap deleted the ROAD-534-add-approved-attribute-to-stories branch October 12, 2023 09:36
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.

Add a state attribute to stories
5 participants