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

Update statuses in detail view for teacher apps #48804

Merged
merged 15 commits into from
Oct 28, 2022

Conversation

megcrenshaw
Copy link
Contributor

@megcrenshaw megcrenshaw commented Oct 24, 2022

This fulfills requirement two in Update Application Statuses.

Statuses in detail view:
image

Statuses in detail view when principal approval is not required:
image

Trying to save before setting registration status:
image

Successfully changing the application statuses:
image

Links

Testing story

Deployment strategy

Follow-up work

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 megcrenshaw requested a review from a team October 25, 2022 19:36
@@ -4,7 +4,7 @@ import {FormGroup} from 'react-bootstrap';
import Select from 'react-select';

// update this to lock scholarships so that scholarship status can't be updated via the UI.
const locked = true;
const locked = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was part of shutting down last year's applications. I've gotten approval to unlock this.

@@ -20,7 +21,7 @@ describe('SummaryTable', () => {
assert(
wrapper.containsMatchingElement(
<tr>
<td>Accepted</td>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing because the field is now Accepted (auto-email)

Copy link
Contributor

@TurnerRiley TurnerRiley left a comment

Choose a reason for hiding this comment

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

Looks great and seems to make the code a lot cleaner/more organized too!

if (this.state.status !== 'incomplete') {
statusesToHide.push('incomplete');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is much more readable now! Creative way of setting this up :D

@dmcavoy
Copy link
Contributor

dmcavoy commented Oct 26, 2022

Hey Meg - I'm realizing that the doc used both "Needs Admin Approval" and "Awaiting Admin Approval" which is my bad. I'm thinking we probably want to just use one. I'm leaning toward standardizing on "Awaiting Admin Approval" what do you think? Also @TurnerRiley what do you think?

@megcrenshaw
Copy link
Contributor Author

Either way works for me. Needs Admin Approval will be shorter on the front-end, but Awaiting Admin Approval sounds nicer imo. Changing it on the front-end will be easy, and it's already awaiting-admin-approval on the backend –– should I go ahead and make it all Awaiting Admin Approval?

@dmcavoy
Copy link
Contributor

dmcavoy commented Oct 26, 2022

Yes lets go ahead and use Awaiting Admin Approval everywhere. I will update the doc to reflect that. Thank you!

@megcrenshaw
Copy link
Contributor Author

Sounds great! Just updated 👍

@TurnerRiley
Copy link
Contributor

Hey Meg - I'm realizing that the doc used both "Needs Admin Approval" and "Awaiting Admin Approval" which is my bad. I'm thinking we probably want to just use one. I'm leaning toward standardizing on "Awaiting Admin Approval" what do you think? Also @TurnerRiley what do you think?

I agree, "Awaiting Admin Approval" sounds good!

@megcrenshaw megcrenshaw merged commit e27850b into staging Oct 28, 2022
@megcrenshaw megcrenshaw deleted the update-teacher-app-statuses branch October 28, 2022 02:16
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.

None yet

3 participants