-
Notifications
You must be signed in to change notification settings - Fork 1
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
Anonymous/walk-in registration #205
Conversation
Hey y'all! A deployment of this PR can be found here: |
I plan on implementing this feature in 3 parts.
Fixed in 04dc661 |
b38b8e8
to
3963457
Compare
Rebased on top of master (fixed tiny conflict) |
</div> | ||
</form> | ||
{{/sidebar}} | ||
<script type="text/javascript"> | ||
formTypeString = "Application"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTF is this a global variable?? Remove the type attribute from the <script>
tag. If you need a global variable, do window.myVariable = x
instead so that it will work in strict mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding it to window
won't work because typescript doesn't recognize additional properties on window
@@ -85,5 +85,8 @@ <h1 class="center">RSVP: {{branch}}</h1> | |||
</div> | |||
</form> | |||
{{/sidebar}} | |||
<script type="text/javascript"> | |||
formTypeString = "Confirmation"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Globals again
client/js/admin.ts
Outdated
@@ -642,6 +645,61 @@ for (let i = 0; i < timeInputs.length; i++) { | |||
timeInputs[i].value = moment(new Date(timeInputs[i].dataset.rawValue || "")).format("Y-MM-DDTHH:mm:00"); | |||
} | |||
|
|||
// Uncheck available confirmation branches for application branch when "skip confirmation" option is selected | |||
function uncheckConfirmationBranches(applicationBranch: string) { | |||
let checkboxes = document.querySelectorAll(`.branch-role[data-name="${applicationBranch}"] .availableConfirmationBranches input[type="checkbox"]`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types will work better here if it's let checkboxes: NodeListOf<HTMLInputElement> = x
. Then you don't need the ugly cast on line 652.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript won't allow me to cast NodeListOf<Element>
to NodeListOf<HTMLInputElement>
client/js/admin.ts
Outdated
(input as HTMLInputElement).checked = false; | ||
} | ||
} | ||
let skipConfirmationToggles = document.querySelectorAll(".branch-role input[type=\"checkbox\"].noConfirmation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with how its not possible.
client/js/admin.ts
Outdated
checkbox.click(); | ||
} | ||
} | ||
let availableConfirmationBranchCheckboxes = document.querySelectorAll(".branch-role fieldset.availableConfirmationBranches input[type=\"checkbox\"]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with how its not possible.
client/js/admin.ts
Outdated
|
||
// Select "skip confirmation" option when "allow anonymous" option is selected | ||
// Hide/show public link when "allow anonymous" is clicked | ||
let allowAnonymousCheckboxes = document.querySelectorAll(".branch-role input[type=\"checkbox\"].allowAnonymous"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with how its not possible.
client/js/admin.ts
Outdated
|
||
// This operation is all or nothing because it will only error if a branch was just made into an Application branch | ||
try { | ||
branchData.allowAnonymous = (branchRoles[i].querySelector("fieldset.applicationBranchOptions input[type=\"checkbox\"].allowAnonymous") as HTMLInputElement).checked; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make fieldset.applicationBranchOptions input[type="checkbox"]
a variable or something somewhere? Seems like it's getting repeated a lot. Or consider running a querySelector()
on that element up front and querySelector()
on the returned element for better performance and readability
@@ -2,7 +2,10 @@ enum FormType { | |||
Application, | |||
Confirmation | |||
} | |||
let formType = window.location.pathname.match(/^\/apply/) ? FormType.Application : FormType.Confirmation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whyyyyyyyyy globals. There has to be a better way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this crime. I literally removed this global.
client/js/application.ts
Outdated
window.location.assign("/"); | ||
|
||
if (unauthenticated) { | ||
(document.querySelector("form") as HTMLFormElement).reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cast not needed here. Typescript can tell that searching for "form" will always return a form element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that the type returned is HTMLFormElement | null
and I want to force it to be not null.
server/routes/api/user.ts
Outdated
next(); | ||
} | ||
else { | ||
response.status(408).json({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a request timeout the appropriate status code for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are a sassyboi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
literally you did this
Still need to test and run it on my machine but overall looks good |
24fbde1
to
2ae1708
Compare
Anonymous/walk-in registration I'm sure I won't regret this
Closes #194
3 options have been added for application branches in the admin panel
/register/<branch-name>
and register new users without accounts.attending=true
without a confirmation branch).#207 is a follow-up that should be implemented to add full functionality for users created in the "anonymous submission" workflow.
This will enable
registration workflow. When all three options are enabled on an application branch.