Skip to content
This repository has been archived by the owner on Nov 6, 2021. It is now read-only.

Adding Impact Stories #415

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Adding Impact Stories #415

wants to merge 11 commits into from

Conversation

fosterv2
Copy link

Resolves #352

Description

Created a model called ImpactStories with title and content attributes. Added a link in the sidebar as described in the issue. Made index, show, new, create, edit, and update controller actions and corresponding views with styling similar to the rest of the project. Only covers an MVP version of the issue, both file upload along with permissions for adding pictures, and sending/accessing impacts stories on the diaper side to be implemented later.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added model, request, and feature test files to test new functionality.

Screenshots

Screen Shot 2020-09-13 at 7 59 59 AM

Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

Great start! I left some comments and change requests. Feel free to reach out if you have any questions.

app/assets/stylesheets/impact_stories.scss Outdated Show resolved Hide resolved
db/migrate/20200905195722_create_impact_stories.rb Outdated Show resolved Hide resolved
db/migrate/20200905195722_create_impact_stories.rb Outdated Show resolved Hide resolved
create_table :impact_stories do |t|
t.string :title
t.text :content
t.integer :partner_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a foreign key constraint here -https://guides.rubyonrails.org/active_record_migrations.html#foreign-keys

This is a really useful thing to add to ensure data integrity is kept. This prevents partner_id from being anything that doesn't match a Partner or if you accidentally delete a Partner

@@ -0,0 +1,11 @@
class CreateImpactStories < ActiveRecord::Migration[6.0]
def change
create_table :impact_stories do |t|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add some database constraints to prevent it at the DB level from having incorrect data. I usually think about the database first when it comes to adding new models. Without these constraints, you could pass invalid data through if you bypass the callbacks using something like .update_column

config/routes.rb Outdated Show resolved Hide resolved
spec/helpers/impact_stories_helper_spec.rb Outdated Show resolved Hide resolved
spec/models/impact_story_spec.rb Outdated Show resolved Hide resolved
spec/factories/impact_stories.rb Outdated Show resolved Hide resolved
Comment on lines 2 to 6

RSpec.describe ImpactStory, type: :model do
it "returns the first part of the content according to the given limit" do
impact_story =
create(:impact_story, title: "For Testing", content: "Making some long content that needs to be shortened according to a given limit")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use https://github.com/faker-ruby/faker to generate randomized data for tests. This is a good idea because sometimes tests pass because it relies on hardcoded data. Using randomized data helps avoid accidentally coupling the data with the actual implementation you are testing.

@scooter-dangle scooter-dangle added the Ruby For Good 🎃 Fall 2020 Ruby for Good coding event, September 2020. label Sep 13, 2020
@seanmarcia seanmarcia changed the base branch from master to main October 1, 2020 22:24
@seanmarcia seanmarcia removed the Ruby For Good 🎃 Fall 2020 Ruby for Good coding event, September 2020. label Jan 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Partner Agencies to Share Their Impact
4 participants