Mandate Constitution Upload#832
Conversation
julianweng
left a comment
julianweng
left a comment
There was a problem hiding this comment.
Thanks for your work Thomas. Left some nits. On a higher level, do you think it could be more elegant to treat constitutions as a foreign key for the club model vs an attribute on a given file upload? It seems like a useful invariant that a club can have only at max one constitution, and there are likely performance benefits as well (it's also easy to prefetch_related or treat this similarly to club photo in terms of our business logic). In addition, especially if we move to this alternative, constitution uploads should be made visually distinct in the UI from normal, non-constitution file uploads and the experience shouldn't feel different from, say, the club photo requirement or field in club edit.
We could also potentially kill the file upload feature altogether in favor of this since I'm not sure if it has any purpose other than constitutions, but that's likely a question for later and we'd want to give clubs time to download their existing files and reupload them as constitutions.
Also, have not gotten a chance to test this locally, backend or frontend (travelling), so will trust that you have!
| isEdit: boolean = false, | ||
| ): boolean { | ||
| // if on edit page and club already has constitution, it's not required | ||
| if (isEdit && club.has_constitution) { |
There was a problem hiding this comment.
I don't think this if branch implements separate behavior on isEdit=true
| enableReinitialize | ||
| validate={(values) => { | ||
| const errors: { email?: string } = {} | ||
| validate={(values: any) => { |
There was a problem hiding this comment.
Are you sure there's no expected schema for values?
|
|
||
| type FilesCardProps = { | ||
| club: Club | ||
| refreshTrigger?: number |
There was a problem hiding this comment.
Is this prop ever being incremented on file upload? We may also want to explore more idiomatic ways of forcing refresh for this card vs. a meaningless number.
| const formData = new FormData() | ||
| formData.append('file', data.file) | ||
|
|
||
| // if filename contains "constitution" then file must be constitution |
There was a problem hiding this comment.
Just confirming that this is fallback logic for file uploads not done within our new upload constitution UI vs a check for files that are already being uploaded through the new UI meant for uploading constitutions.
| <p className="has-text-weight-semibold"> | ||
| Important: If you choose to upload your constitution from this | ||
| tab, please ensure the file name contains the word "constitution" | ||
| to automatically mark it as a constitution file. |
There was a problem hiding this comment.
Refer to the above comment but I think this process is too confusing / ambiguous and if we can't avoid this, we should just have a single unambiguous place where uploading any valid word or pdf doc would idempotently update a club's constitution
| def is_wharton(self): | ||
| return any(badge.label == "Wharton Council" for badge in self.badges.all()) | ||
|
|
||
| @cached_property |
There was a problem hiding this comment.
Do we invalidate this cache on file upload or is this implicit in the Django decorator?
2 participants
This PR mandates that clubs upload their constitutions. Uploading a constitution file can be done via resource tab, in which case the file name should at least contain
constitutionas a substring. The file upload section could potentially be used to upload files that aren't intended to be the constitution. One can also upload a constitution file via theClubEditCardon/create/.../renew/.../edit/routes where we assume the user definitely intends to upload a constitution.On the resource tab, you'll see some warning text that is conditionally rendered for clubs that don't satisfy the constitution requirement.
NOTE: club mission word limit was implemented in an earlier commit not in this PR
Closes #825