Confirmation refactor #214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lification of redirection logic #206
|
Hey y'all! A deployment of this PR can be found here: |
ehsanmasdar
commented
May 8, 2018
server/middleware.ts
Outdated
| @@ -205,8 +205,9 @@ export function branchRedirector(requestType: ApplicationType): (request: expres | |||
| response.redirect(`/confirm/${encodeURIComponent(user.confirmationBranch.toLowerCase())}`); | |||
| return; | |||
| } | |||
| // TODO why is this !user.attending? | |||
| if (questionBranches.indexOf(branchName.toLowerCase()) === -1 && !user.attending) { | |||
There was a problem hiding this comment.
@bunsenmcdubbs @petschekr Can you clarify why !user.attending is here.
When we accept a user we assign them to a specific confirmation branch (and only one confirmation branch). Each confirmation branch is an option in the state dropdown. Instead of sending an acceptance email for the application branch, we send a pre-confirm email for the confirmation branch. This allows us to alter the message text for each branch. Waitlist/Rejected edge cases: Waitlist can operate as a normal confirmation branch if we have people confirm their spot on the waitlist. Rejected can be implemented by a simple no-op confirmation branch that does not require any user action (this is also necessary for supporting the walk-up feature). A flag on each branch can indicate if this confirmation represents "success" or "failure", for the purposes of ensuring we only check-in accepted students. The only reduction in functionality here is that users can no longer choose from multiple confirmation branch. In my opinion, this isn't necessary for us and only lead to confusion when different confirmation branches had different deadlines.
ehsanmasdar
force-pushed
the
walk-up-fixes
branch
from
d929cb0
to
718422e
Compare
ehsanmasdar
changed the title
petschekr
added
enhancement
Closed
Fixes lowercase branch names crashing the server when navigating to the apply or confirm forms
ehsanmasdar
commented
Jun 5, 2018
server/middleware.ts
Outdated
| @@ -229,7 +236,7 @@ export function branchRedirector(requestType: ApplicationType): (request: expres | |||
| } | |||
| } | |||
| if (requestType === ApplicationType.Confirmation) { | |||
| if (request.params.branch !== user.confirmationBranch ) { | |||
| if (!user.accepted || branchName.toLowerCase() !== (user.confirmationBranch || "").toLowerCase()) { | |||
There was a problem hiding this comment.
@petschekr user.accepted is not an indicator of if the user has been assigned a confirmation branch, it's better to check if user.confirmationBranch is defined.
There was a problem hiding this comment.
@ehsanmasdar Thanks for pointing that out. Does everything in 2e2bd7d make sense now with the distinction between confirmed and attending?
Fixes missing space in formatted application submission success message and removes "We look forward to seeing you" because the message is also shown for rejected applicants.
petschekr
approved these changes
Jun 5, 2018
There was a problem hiding this comment.
@ehsanmasdar I made a bunch of small fixes to this PR. Could you please look them over before we merge this? Overall, this is great, resolves a ton of issues, makes the code more maintainable, and is simpler to use. Thanks!
- Scroll to top of page on successful submission - Properly slugify anonymous submission links in the admin panel - Set a title on branch names in the admin panel that might be truncated due to limited column width
Breaking API and UX changes for admins and users
|
@petschekr Your changes look good. I'd like to get this reviewed at least one more time considering the scope of changes, @kexin-zhang could you take a look. |
petschekr
added
the
blocking
kexin-zhang
approved these changes
Jun 10, 2018
rahulrajus
pushed a commit
that referenced
this pull request
Jul 22, 2021
Confirmation refactor SEMVER-MAJOR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we accept a user we assign them to a specific confirmation branch (and only one confirmation branch). Each confirmation branch is an option in the state dropdown.
Instead of sending an acceptance email for the application branch, we send a pre-confirm email for the confirmation branch. This allows us to alter the message text for each branch.
Waitlist/Rejected edge cases:
Waitlist can operate as a normal confirmation branch if we have users confirm their spot on the waitlist. Rejected can be implemented by a simple no-op confirmation branch that does not require any user action (this is also necessary for supporting the walk-up feature). A flag on each branch can indicate if this confirmation represents "success" or "failure", for the purposes of ensuring we only check-in accepted students.
The only reduction in functionality here is that users can no longer choose from multiple confirmation branch.
This PR implements the above changes. Some notes/other additions:
attendingis nowconfirmed)accepted.acceptednow simply indicates if the user has been assigned a confirmation branch that we define as anacceptedbranch. A user could, for example, confirm to the waitlist but not be accepted.a. Application branches no longer include a whitelist of confirmation branches
b. Application branch automation now configurably can automatically set a user's confirmation decision
c. Confirmation branches now have a flag to auto-confirm. This functions like a no-op, the user never recieves a post-confirm email and cannot edit/submit the branch.
d. Application branches no longer include a whitelist of confirmation branches
e. Refactor the search filters to fit this new model.
Fixes #110, fixes #117, fixes #150, fixes #156, fixes #206, fixes #208, fixes #209, fixes #211