Set correct application status on submit #48949

megcrenshaw · 2022-11-01T03:07:05Z

We have a new status for teacher applications: awaiting_admin_approval. The status itself was added in #48804.

There are two things that need to happen with this new status:

When a teacher submits their application, its status used to be unreviewed. Now, it should only be unreviewed if the RP set that admin approval is "selective," i.e. they manually decide for each application if it requires admin approval. Otherwise, it should be awaiting_admin_approval. << This PR addresses this part.
A program manager can change the admin approval requirement on an individual application. If they do change it, the status should change from unreviewed to awaiting_admin_approval if it is now required, and it should change from awaiting_admin_approval to unreviewed if it is no longer required. << A future PR will address this part.

Links

Testing story

I added unit tests for each change. I tested manually that I can still save and submit the teacher application. I will test manually on the adhoc that the statuses are being set correctly once the second task mentioned above is complete, and both are merged into staging.

Deployment strategy

Teacher applications are still closed for this year; the risk is low to merge now and test manually on the adhoc.

Follow-up work

The second task mentioned above will be addressed in a separate PR.

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

megcrenshaw · 2022-11-02T01:04:52Z

apps/src/code-studio/pd/form_components_func/FormController.jsx

@@ -114,8 +114,6 @@ const FormController = props => {
  const [showDataWasLoadedMessage, setShowDataWasLoadedMessage] = useState(
    applicationId
  );
-  const applicationStatusOnSave = 'incomplete';
-  const applicationStatusOnSubmit = 'unreviewed';


These changed because now there are two potential statuses on submit –– instead, let the backend figure out which status it should be

megcrenshaw · 2022-11-02T01:14:22Z

dashboard/test/controllers/api/v1/pd/application/teacher_applications_controller_test.rb

      refute TEACHER_APPLICATION_CLASS.last.response_scores
      assert_response :created
    end

-    test 'autoscores and queues email once application is submitted' do
-      application_hash = build :pd_teacher_application_hash_common, :csp
+    test 'autoscores and queues emails on submit when approval is required' do


There's a lot of repeated code in these new tests. I wrote up a .each way to reduce duplication, but I found the readability hard. Please push back if you think this is too much!

I feel like prioritizing readability might make sense since we are shifting lots of terminology around statuses and desired behaviors so having each one more explicitly laid out might be better as we home in on the desired outcome? That's just a thought though, I'm not dug in on this by any means!

I'm on the same page –– thanks!

…utomatic-status

TurnerRiley

Looks fantastic! I had a couple comments but this looks good to merge!

TurnerRiley · 2022-11-03T17:00:20Z

dashboard/app/models/pd/application/application_base.rb

@@ -61,12 +61,12 @@ class ApplicationBase < ApplicationRecord
    validate :status_is_valid_for_application_type
    validates_presence_of :type
    validates_presence_of :user_id, unless: proc {|application| application.application_type == PRINCIPAL_APPROVAL_APPLICATION}
-    validates_presence_of :status, unless: proc {|application| application.application_type == PRINCIPAL_APPROVAL_APPLICATION}


Why is this portion of code being removed? Will this allow both the teacher applicant and the associated admin/principal/school leader to be able to see the status of the same application?

This line ensures there is a status for each application record (so both teacher applications and principal approval applications, since both inherit from application_base). It doesn't affect who can see what.

We are already checking status_is_valid_for_application_type, so that line covers it because there are statuses defined for all application_base records here https://github.com/code-dot-org/code-dot-org/blob/staging/dashboard/app/models/pd/application/application_base.rb#L171

(this method is overridden in the teacher application model)

I'm guessing this line was here before we had the more specific validation via status_is_valid_for_application_type

Oh gotcha, thank you!

Just to double check, would any existing entries in this table now be invalid?

Good question. We are removing a validation -- would that spark new validation errors? I can't imagine it would, but is there a better way to check?

It would only cause problems if someone tried to save the old application and it didn't have a status. You could query the DB to see if any row is in that situation but if you're reasonably confident there isn't one, that's good enough for me.

Ah I see. But if it did not have a status, I would think nothing would be triggered because this validation is now completely removed? It's already passing the other status validation above too.

Just ran this on production, so we're good here 👍

[production] dashboard > Pd::Application::ApplicationBase.where(status: nil) => #<ActiveRecord::Relation []>

TurnerRiley · 2022-11-03T17:28:42Z

dashboard/test/controllers/api/v1/pd/application/teacher_applications_controller_test.rb

      refute TEACHER_APPLICATION_CLASS.last.response_scores
      assert_response :created
    end

-    test 'autoscores and queues email once application is submitted' do
-      application_hash = build :pd_teacher_application_hash_common, :csp
+    test 'autoscores and queues emails on submit when approval is required' do


I feel like prioritizing readability might make sense since we are shifting lots of terminology around statuses and desired behaviors so having each one more explicitly laid out might be better as we home in on the desired outcome? That's just a thought though, I'm not dug in on this by any means!

TurnerRiley · 2022-11-03T17:36:57Z

dashboard/test/controllers/api/v1/pd/application/teacher_applications_controller_test.rb


      sign_in @applicant

-      put :create, params: @test_params
+      put :create, params: {form_data: application_hash}


Nit?: I noticed {form_data: application_hash} a couple times replacing @test_params, would it be worth defining 2 different set of params at the top based on their differences so that this can be re-used more easily? If that wouldn't work or would end up being too cumbersome then please ignore this!

Good call. I just looked to see if we could define something for {form_data: application_hash} at the top. We're defining different application_hash variables inside each test based on the regional parter we create inside the test. BUT it looks like we could just have two regional partners created at the top and then create application hashes based on those regional partners! I'll look into that 👍

I made this update!

bethanyaconnor · 2022-11-04T12:50:53Z

dashboard/app/controllers/api/v1/pd/application/teacher_applications_controller.rb

@@ -11,6 +11,17 @@ def new_form
      )
    end

+    def new_status
+      return 'incomplete' if ActiveModel::Type::Boolean.new.cast(params[:isSaving])


bethanyaconnor · 2022-11-04T12:56:07Z

dashboard/app/controllers/api/v1/pd/forms_controller.rb

-    if status
-      @application.status = status
-    end
+    form.status = new_status if form.is_a? Pd::Application::TeacherApplication


Is this fixing a bug? It makes sense but I'm not sure how it fits into the rest of this PR

This should only be run for teacher apps because it's not relevant for principal apps (or any other app type that inherits from form).

The previous code was a bit janky (I wrote it so I can say that tehe) because the local status variable was only set with teacher apps

bethanyaconnor · 2022-11-04T13:01:03Z

dashboard/app/models/pd/application/application_base.rb

      define_method(:"#{attribute}?") do
        status == attribute
      end
    end

    def set_applied_date
-      self.applied_at = Time.now if applied_at.nil? && unreviewed?
+      self.applied_at = Time.now if applied_at.nil? && (unreviewed? || awaiting_admin_approval?)


You check or either unreviewed? or awaiting_admin_approval? a couple times. Would it be worth adding a helper method called something like awaiting_action? (my knowledge in this space is limited so feel free to wordsmith). It's also only a couple of places so it might not be worth it. Thoughts?

That's a good call. Maybe status_on_submit? Or submitted_status?

This line could also be !incomplete? I'll think on it

Updated to status_on_submit? –– I'm sure we'll be using this more in the future. Good call!

bethanyaconnor

Looks great!

Meg Crenshaw added 13 commits October 31, 2022 08:16

Add auto-email text and change order

8ce14b5

Change all instances to 'Awaiting Admin Approval'

771719b

Set applied_at field if awaiting_admin_approval

61f3cdb

Make form variable consistent

03a98e6

Run on_successful_create if app is awaiting_admin_approval

44bfcbc

Change the way status is passed from the frontend

b0df397

Check principal approval required before setting status

ab04839

Fix isSaving param

601cf4b

Fix isSaving param

ed86bab

Remove changes on changing principal approval

6eb060c

Update allowing admin emails based on new status

b86185b

Reduce repeated code in test

d28f6b7

Refactor tests

20a2056

megcrenshaw commented Nov 2, 2022

View reviewed changes

Remove unneccessary line

f28acda

megcrenshaw commented Nov 2, 2022

View reviewed changes

megcrenshaw changed the title ~~Admin approval automatic status~~ Set correct application status on submit Nov 2, 2022

megcrenshaw force-pushed the admin-approval-automatic-status branch from ed8e465 to 44789f6 Compare November 2, 2022 17:13

Update model tests

d645cc5

megcrenshaw force-pushed the admin-approval-automatic-status branch from 44789f6 to d645cc5 Compare November 2, 2022 18:11

megcrenshaw requested a review from a team November 2, 2022 18:31

Merge remote-tracking branch 'upstream/staging' into admin-approval-a…

dd029d4

…utomatic-status

TurnerRiley approved these changes Nov 3, 2022

View reviewed changes

Refactor tests to use instance variables

5722b43

bethanyaconnor reviewed Nov 4, 2022

View reviewed changes

bethanyaconnor approved these changes Nov 4, 2022

View reviewed changes

Use helper for unreviewed and awaiting_admin_approval pairing

8bdcfa9

bethanyaconnor approved these changes Nov 4, 2022

View reviewed changes

megcrenshaw merged commit a71f81b into staging Nov 4, 2022

megcrenshaw deleted the admin-approval-automatic-status branch November 4, 2022 19:42

megcrenshaw mentioned this pull request Nov 4, 2022

Update status based on admin approval #49007

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set correct application status on submit #48949

Set correct application status on submit #48949

megcrenshaw commented Nov 1, 2022 •

edited

megcrenshaw Nov 2, 2022

megcrenshaw Nov 2, 2022

TurnerRiley Nov 3, 2022

megcrenshaw Nov 3, 2022

TurnerRiley left a comment

TurnerRiley Nov 3, 2022

megcrenshaw Nov 3, 2022

megcrenshaw Nov 3, 2022

TurnerRiley Nov 3, 2022

bethanyaconnor Nov 4, 2022

megcrenshaw Nov 4, 2022

bethanyaconnor Nov 4, 2022

megcrenshaw Nov 4, 2022

megcrenshaw Nov 4, 2022

TurnerRiley Nov 3, 2022

TurnerRiley Nov 3, 2022

megcrenshaw Nov 3, 2022

megcrenshaw Nov 4, 2022

bethanyaconnor Nov 4, 2022

bethanyaconnor Nov 4, 2022

megcrenshaw Nov 4, 2022

bethanyaconnor Nov 4, 2022

megcrenshaw Nov 4, 2022 •

edited

megcrenshaw Nov 4, 2022

bethanyaconnor left a comment

Set correct application status on submit #48949

Set correct application status on submit #48949

Conversation

megcrenshaw commented Nov 1, 2022 • edited

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

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

TurnerRiley 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megcrenshaw Nov 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor left a comment

Choose a reason for hiding this comment

megcrenshaw commented Nov 1, 2022 •

edited

megcrenshaw Nov 4, 2022 •

edited