Professional Development Feature

[perryzjc]

Pivotal Tracker Link

What this PR does:

This pull request fixes|implements implements professional development feature for admins to create PD workshops and add teachers to each workshop

Include screenshots, videos, etc.

CURD for PD

CURD for PD Registrations

Who authored this PR?

1. Perry (Jingchao) Zhong (integrate frontend + backend, frontent mockup)
2. Michael Tao (integrate frontend + backend)
3. Jackson Xu (some backend implementation)

How should this PR be tested?

• Is there a deploy we can view?
• What do the specs/features test?

1. Test Basic CURD for PD and PD registrations
2. Test UX workflow & value validations

• Are there edge cases to watch out for?

Are there any complications to deploying this?

1. Run rails db:migrate

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])

[codecov]

Codecov Report

Attention: Patch coverage is 90.19608% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 86.70%. Comparing base (809ce23) to head (dbeb705).
Report is 44 commits behind head on main.

❗ Current head dbeb705 differs from pull request most recent head 417adea. Consider uploading reports for the commit 417adea to get more accurate results

Files	Patch %	Lines
app/controllers/pd_registrations_controller.rb	83.78%	6 Missing ⚠️
...ontrollers/professional_developments_controller.rb	93.54%	2 Missing ⚠️
app/models/pd_registration.rb	90.90%	1 Missing ⚠️
app/models/professional_development.rb	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   86.03%   86.70%   +0.66%     
==========================================
  Files          20       24       +4     
  Lines         709      797      +88     
==========================================
+ Hits          610      691      +81     
- Misses         99      106       +7     

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

[ArushC]

@perryzjc Overall this looks good. Everything works as intended. But there were two main things that I noticed.

1. There seems to be an extreme lack of comments for the number of lines of code that were written. I don't think Michael really cares, and if it's too much work to write comments then don't put any. But some more comments would be helpful to include to understand what's going on at a high level, especially in some of the 100+ line code files that you added.

2. You are missing some "sad path" tests according to the CodeCov warnings (see below). The sad path stuff obviously works based on the code that I see. But it shouldn't be too hard to get ChatGPT to right a few quick tests.

Here are specific comments:

1. def set_pd_registration
   @pd_registration = PdRegistration.find(params[:id])
   rescue ActiveRecord::RecordNotFound
   redirect_to professional_development_path, alert: "Registration not found." ---> THIS PATH IS MISSING TESTING (probably needs to be tested using RSpec, since frontend won't go to an invalid path)

2. def set_professional_development
   @professional_development = ProfessionalDevelopment.find_by(id: params[:professional_development_id])
   unless @professional_development
   redirect_to professional_developments_path, alert: "Professional Development not found." --> THIS PATH IS MISSING TESTING (also needs to be tested using RSpec, since frontend won't go to an invalid path)

3. def destroy
   if @professional_development.destroy
   redirect_to professional_developments_url, notice: "Professional development deleted successfully."
   else
   redirect_to professional_developments_url, alert: "Failed to delete professional development." --> UNNECESSARY if/else clause

There is no reason why deleting a professional development should fail. In all other places in the app where stuff is destroyed there is no error checking to see if deleting something fails. So remove the if/else stuff.

4. At the bottom of app/views/professional_developments/_form.html.erb, I noticed that you basically just copy-pasted the script from one of the other views. Is there a way that we can DRY out this code and maybe put the script in some third location where it can be loaded in for both the views?

5. def registration_open
   @professional_development.registration_open ? "Yes" : "No"

   Is this function even necessary? It doesn't seem to be used anywhere in the code...

[ArushC]

Also nooo.... there's a merge conflict :(

[fp456]

Line 10 not tested

[fp456]

This path was not tested

[fp456]

if possible, test

[fp456]

missing test

[fp456]

missing test, i think arush mentioned this already?

[fp456]

test if possible

[fp456]

Looks good so far! Just some extra testing needed, which is necessary for a large PR like this. Goood job! 🙌

[ArushC]

@perryzjc what's the progress on this PR. Is it ready to be merged yet?