-
Notifications
You must be signed in to change notification settings - Fork 13
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
positions: Add Position Postings #577
base: dev
Are you sure you want to change the base?
Conversation
Per WPI-LNL#562, this creates a new django app for the position postings. The thought process behind the position flow is: 1. Exec posts a position 2. Active members can see that there are open positions, and can view them on the DB and apply with 1 click (side thought on displaying -- I was thinking something similar to the CC report reminders but on the main page) 3. Applicants are given a form to complete if provided by exec, and exec can see the applicants event/training history in the position view 4. Exec votes and does exec-y things to pick the person, who then gets a congratulatory email and the position is marked as closed. Writing this out, I need to add a field to the `Position` model to mark if the position has been filled.
This commit will add the following templates: - position_list - position_detail It will update the following models: - Position I'm debating just getting rid of the ApplicationInstance model entirely and letting exec track applications through their own external tools.
Some positions won't have any reports (e.g exec board positions posted so that potential candidates know what the job entails).
Please open all new pull requests against the dev branch. Thank you! |
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.
There's a lot of good progress here, but there's still quite a ways to go. For one thing, you need to have test cases written for this. Please also consider writing up a User Guide on how to view the open positions and apply (if applicable).
Previously, the template would error trying to reverse to the accounts detail page for None. It now defaults to "Exec Board" as a whole, unless a specific officer is set.
This will change the `closes` field in the positions app to a DateTime field, and the `application_form` field to be a URLField.
This will test every view for access control, as well as the `is_closed` method on Positions
Some positions do not require applications (e.g Executive board positions). See WPI-LNL#577 (comment) Signed-off-by: Cara Salter <[email protected]>
@tnurse18 Should be all set -- I'm going to write some documentation while I wait for a review. |
Signed-off-by: Cara Salter <[email protected]>
This adds the standard CRUD permissions for Positions, as well as a custom permission for "Apply", that should be granted to Active members |
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.
Sorry it took so long to complete my review, but I finally have some feedback for you! Overall this is really coming together quite well. You've done an excellent job here and should be really proud of what you've accomplished so far. In addition to the comments I’ve made throughout, here are a couple more things to consider:
- Now that your models seem to be just about ready to go, if you’re comfortable squashing down your migrations into one, that would be great. If not, I can take care of that for you before we merge to master.
- Still missing documentation for adding a position.
@@ -80,6 +81,9 @@ def admin(request, msg=None): | |||
datetime_end__gte=(now - datetime.timedelta(hours=3))).distinct() | |||
context['selfcrew_events'] = selfcrew_events | |||
|
|||
open_positions = Position.objects.filter(closes__gte=timezone.now()).count() |
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.
This also excludes listings that don't have applications that close
Apply for a position | ||
==================== | ||
|
||
Active members can now see a list of open leadership positions created by the |
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 title of the page is "Apply for a position", so maybe we should mention that they can do that here as well? Something like:
Active members can now view and apply for leadership opportunities online. New positions are posted regularly by the Executive Board.
----------------- | ||
To view a list of open positions, go to the "Members" dropdown and find the | ||
"Open Positions" link. Clicking that link will bring you to a list of positions | ||
that are currently looking for interested members. |
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.
that are currently looking for interested members. | |
that we are currently looking for candidates for. |
should be created every time one needs to be filled. | ||
""" | ||
name = models.CharField(verbose_name="Position Name", max_length=64, | ||
null=False, blank=False) |
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.
This is redundant. Null and blank are False by default so they technically don't need to be specified. Not necessarily a problem, just reads a bit nicer in my opinion without them.
self.helper.add_input(Submit('submit', 'Submit')) | ||
class Meta: | ||
model = Position | ||
fields = ('name', 'description', 'position_start', 'position_end', |
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.
- For the
description
field, you may want to consider using something like the Pagedown widget. - For the
closes
field you should definitely use the SplitDateTimeField. -
reports_to
should probably be a AutoCompleteSelectField.
There are tons of examples in other modules that you can look at if you need to.
<table class="table"> | ||
<tr> | ||
<th>Description</th> | ||
<td>{{ object.description }}</td> |
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 would consider using the markdown filter here. When I tried this out, I tried adding a list as part of my description, but that didn't display well.
</tr> | ||
<tr> | ||
<th>Applications Close</th> | ||
<td>{% if object.closes %}{{ object.closes }}{% else %}Doesn't |
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 would probably hide this row if there isn't an application.
<td><a href="{% url 'positions:detail' pos.id %}">{{ pos }}</a></td> | ||
<td>{{ pos.position_start }}</td> | ||
<td>{{ pos.position_end }}</td> | ||
<td>{% if pos.closes %}{{ pos.closes }}{% else %}Does not |
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.
If there isn't an application, maybe something like Not applicable
would be better here?
permission_required="positions.add_position" | ||
|
||
def get_success_url(self): | ||
return reverse("accounts:me") |
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.
This seemed a bit odd in my opinion and it definitely caught me by surprise. I think the user would be expecting to end up on either the position detail page or the open positions list.
|
||
class ListPositions(PermissionRequiredMixin, LoginRequiredMixin, generic.ListView): | ||
model = Position | ||
queryset = Position.objects.filter(closes__gte=datetime.datetime.now()) |
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 should definitely use timezone here instead
queryset = Position.objects.filter(closes__gte=datetime.datetime.now()) | |
queryset = Position.objects.filter(closes__gte=timezone.now()) |
Closes #562. Allows officers to post positions within the DB and open them to
all active members. Active members are given the chance to apply via an external
form.