CS169 Iteration 1 Pull Request #291

ArushC · 2024-03-01T01:52:57Z

Pivotal Tracker Link

We decided to include the links to each of the pull requests that were made over the course of this iteration. See below. PRs are listed in chronological order, from earliest to most recent:

Change reason to denial reason: cs169#29

Add to field: cs169#30

Add email resend flash message: cs169#31

Add request more info feature as admin: cs169#32

Edit redirect to use current path: cs169#33

Fix email template reseed problem: cs169#34

What this PR does:

Implements iteration 1 features, including request more information button, the "to" field in the email template, and the flash message on resending emails. We also changed "reason" to "denial_reason" and fixed the duplicate email entry problem that occurs when you run "rails db:seed" after running migrations.

Include screenshots, videos, etc.

Who authored this PR?

Arush Chhatrapati
Perry (Jingchao) Zhong
Michael Tao
Jackson Xu
Raz Turcu

How should this PR be tested?

See cucumber and RSpec tests, and view the latest deployment at https://sp24-01-bjc-teachers-d0e972207d82.herokuapp.com/dashboard.

Is there a deploy we can view?
https://sp24-01-bjc-teachers-d0e972207d82.herokuapp.com/dashboard
What do the specs/features test?
To make sure everything works as expected
Are there edge cases to watch out for?
No

Are there any complications to deploying this?

Basically no, but if there are any potential risks, the update feature of reseed might overwrite the old email templates in the database if the hashed values are the same.

Run "rails db:migrate" and "rails db:seed".

Checklist:

Has this been deployed to a staging environment or reviewed by a customer?
Tag someone for code review (either a coach / team member)
I have renamed the branch to match PivotTracker's suggested one (necessary for BlueJay) (e.g. michael/12345-add-new-feature Any branch name will do as long as the story ID is there. You can use git checkout -b [new-branch-name])

…eason

…ration

…emplate

…s loaded on each run

reason to denial reason

…l template CRUD routes

Allow manually triggering specs

…flash-message Feature/187044317 add email resend flash message

Add request more info feature as an admin

187044314 add to field

…me-email-redirect edit redirect to use current path

…safety" This reverts commit 9fb64e9.

Fix email template reseed problem upon updates/deletion

…-reseed Refract implementation & test cases for email template seed

perryzjc · 2024-03-02T20:18:20Z

A new PR just merged into cs169's main branch, which fixes the implementation & tests for email reseed problem we discussed last Friday (Mar 1).

fp456 · 2024-03-04T19:55:02Z

Looks good so far but we can add more testing, especially for "Add request more info feature as admin".

cycomachead

Some initial comments

cycomachead · 2024-03-05T17:36:50Z

app/controllers/teachers_controller.rb

@@ -146,7 +146,8 @@ def deny
    @teacher.denied!
    if !params[:skip_email].present?
      # TODO: Update dropdown to select the email template.
-      TeacherMailer.deny_email(@teacher, params[:reason]).deliver_now
+      puts params[:denial_reason]


Please clean this up. :)

cycomachead · 2024-03-05T17:56:33Z

app/views/email_templates/_form.html.erb

@@ -1,7 +1,10 @@
+<%# This part of the page is the email template form %>


I would delete a comment like this.

cycomachead · 2024-03-05T18:03:49Z

db/migrate/20240220192015_change_to_field_for_form_submission.rb

@@ -0,0 +1,5 @@
+class ChangeToFieldForFormSubmission < ActiveRecord::Migration[6.1]


We should delete this file, or move it to a lib folder.

cycomachead · 2024-03-05T18:06:33Z

db/migrate/20240218005604_add_to_field_to_email_template.rb

+class AddToFieldToEmailTemplate < ActiveRecord::Migration[6.1]
+  def change
+    add_column :email_templates, :to, :string
+    #below is the default prepopulated 'to' field value,


I don't think Rails migrations should be used to change data, only the schema.

We should move this to the data migrating to a lib file or just to document what admins might need to do.

During the class discussion, Michael thought we could just skip the migration for this part; he could just manually update the field.

At the new pull request, we may want to include this info for the migration note for the new PR, e.g. optionally to change the to field of email template in that format

cycomachead · 2024-03-05T18:08:55Z

db/seeds.rb

@@ -2,7 +2,12 @@

 require_relative "seed_data"

-SeedData.emails.each { |email| EmailTemplate.find_or_create_by(email) }
+SeedData.emails.each do |email_attrs|
+  # Based on what Professor Ball mentioned, 'title' is the unique identifier for each EmailTemplate


Comments like this read a little funny. :)

{ title: email_attrs[:title] } can just be passed directly to the find function

cycomachead · 2024-03-05T18:09:54Z

spec/fixtures/teachers.yml

+  more_info: ''
+  application_status: Denied
+  school: berkeley
+  education_level: -1


This file should end with a new line.

cycomachead · 2024-03-05T18:13:40Z

app/views/main/_deny_modal.html.erb

+            case "deny":
+                newAction = `/teachers/${teacherId}/deny`;
+                newTitle = 'Deny ' + teacherName;
+                inputName = 'denial_reason'; // The dynamic part for the input's name attribute


Delete the comment at the end of the line.

codecov · 2024-03-08T19:34:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.77%. Comparing base (0f79f52) to head (df52d67).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   81.95%   85.77%   +3.82%     
==========================================
  Files          20       20              
  Lines         654      668      +14     
==========================================
+ Hits          536      573      +37     
+ Misses        118       95      -23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cycomachead and others added 30 commits February 12, 2024 11:02

Allow manually triggering specs

d3b2f5e

change reason to denial reason

c343618

updated relevant deny cucumber test to use denial_reason instead of r…

754d4c7

…eason

rubocop fix

092cce2

migration adds 'to' feld to email template database

2bab1d0

added to field to view and added a file I forgot to add from prev mig…

b1173d2

…ration

update backend to send to all emails specified in To field in email t…

870491a

…emplate

fixed rspec test failures and rubocop by updating seed data which get…

c231e07

…s loaded on each run

added test with barney the purple dinosaur

be305ef

rubocop fix

2088b07

Add request reason email template

4ce4a2d

Fix reason naming

15da9b5

Add request reason to liquid template engine

5da7ca6

Merge pull request #29 from cs169/187044290-reason-to-denial-reason

2ee19ed

reason to denial reason

fix request info email template

725ae84

Add rspec test for request reason email

f99087d

change prepopulated TO field for form submission email

7a8e473

added validations and more descriptive, useful error messages to emai…

1b42b58

…l template CRUD routes

rubocop fixes

96267f9

added cucumber tests and added untracked migration file

96c18ec

add flash message to resend welcome email

177c1c6

Merge pull request #26 from cs169/cycomachead-patch-1

4862e0b

Allow manually triggering specs

add flash message after welcome resend

3aefd1e

Added button UI, modified UI logic, added comments for backend

9248494

Merge pull request #31 from cs169/feature/187044317-add-email-resend-…

928c57b

…flash-message Feature/187044317 add email resend flash message

Linked UI to email template

d4db5b3

Update UI logic for request info and pass the test

21718e5

Merge pull request #32 from cs169/187044299/feature/request_reason

325fb70

Add request more info feature as an admin

rebase against main and fix merge conflict in seed data

ac57d48

Merge pull request #30 from cs169/187044314-add-to-field

d9d096d

187044314 add to field

JacksonXu33 and others added 16 commits February 27, 2024 10:52

edit redirect to use current path

c64216f

Merge pull request #33 from cs169/feature/187044317-edit-resend-welco…

d1a4b1d

…me-email-redirect edit redirect to use current path

Fix email template reseed problem

9eeb0dc

Add rspec test for email reseed behavior

82c5475

Refract rspec test of email reseed to be more modular

d00c889

Add RSpec db check to ensure tests use bjc_teachers_test for safety

9fb64e9

Revert "Add RSpec db check to ensure tests use bjc_teachers_test for …

2f30dc1

…safety" This reverts commit 9fb64e9.

Adjust Rspec test to avoid purge old email

7464961

Update email reseed implementation to avoid purge old email templates

755a96b

Merge pull request #34 from cs169/187113121/bugfix/email_reseed

771324f

Fix email template reseed problem upon updates/deletion

Refract Rspec test for email reseed

dad60c5

finish refract email reseed test cases

54bd3fd

move email reseed test cases to models folder

5bffa76

Fix rubocop check

ba4a96b

Change the way of hashing for updating email template seed

0fe0be5

Merge pull request #35 from cs169/187156425/bugfix/refract-test-email…

df52d67

…-reseed Refract implementation & test cases for email template seed

cycomachead requested changes Mar 5, 2024

View reviewed changes

ArushC closed this Mar 8, 2024

ArushC mentioned this pull request Mar 8, 2024

187186207/iter1 golden repo fixes #292

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CS169 Iteration 1 Pull Request #291

CS169 Iteration 1 Pull Request #291

ArushC commented Mar 1, 2024 •

edited

Loading

perryzjc commented Mar 2, 2024

fp456 commented Mar 4, 2024

cycomachead left a comment

cycomachead Mar 5, 2024

cycomachead Mar 5, 2024

cycomachead Mar 5, 2024

cycomachead Mar 5, 2024

perryzjc Mar 8, 2024 •

edited

Loading

cycomachead Mar 5, 2024

cycomachead Mar 5, 2024

cycomachead Mar 5, 2024

codecov bot commented Mar 8, 2024

		@@ -1,7 +1,10 @@
		<%# This part of the page is the email template form %>

		@@ -0,0 +1,5 @@
		class ChangeToFieldForFormSubmission < ActiveRecord::Migration[6.1]

CS169 Iteration 1 Pull Request #291

CS169 Iteration 1 Pull Request #291

Conversation

ArushC commented Mar 1, 2024 • edited Loading

What this PR does:

Include screenshots, videos, etc.

Who authored this PR?

How should this PR be tested?

Are there any complications to deploying this?

Checklist:

perryzjc commented Mar 2, 2024

fp456 commented Mar 4, 2024

cycomachead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perryzjc Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2024

Codecov Report

ArushC commented Mar 1, 2024 •

edited

Loading

perryzjc Mar 8, 2024 •

edited

Loading