Introduce separate views for Get Involved submissions #2492
Conversation
Django default date format uses AP style month abbreviations, which aren't available in strftime. Plus showing the full month is marginally more user-friendly as space is not a concern in this case.
|
@pbanaszkiewicz Sorry, I forgot to request your review when I opened this PR (though I know you've been away so maybe it makes no difference!). Could you prioritise reviewing this when you have time please? |
There was a problem hiding this comment.
@elichad I added some comments, mostly I'm concerned about the post() method used in 1 or 2 views. The rest is minor things.
| <p> | ||
| Provide details of your Get Involved activity. Your submission will be reviewed within 7-10 days. | ||
| You can track your progress by returning to the <a href="{% url 'training-progress' %}">training progress</a> page. | ||
| </p> | ||
| <p> | ||
| Only submit an activity that you have already completed. | ||
| </p> | ||
| <p> | ||
| If submitting a GitHub contribution, please be sure your contribution is to a repository in one of the | ||
| <a href="https://docs.carpentries.org/topic_folders/communications/tools/github_organisations.html"> | ||
| GitHub organisations owned by The Carpentries | ||
| </a>. | ||
| </p> | ||
| {% crispy form %} |
There was a problem hiding this comment.
Can this be replaced with the following?
{% include "includes/get_involved_form_embedded.html" with get_involved_form=form %}
There was a problem hiding this comment.
It can, though the ...embedded.html file is a holdover from earlier experiments that I had meant to delete! Do you still think it would be better to use the include even though this template is fairly simple?
There was a problem hiding this comment.
@elichad If the "embedded" file is going away then there's no need to use include here.
| rv = self.client.get(reverse("getinvolved_delete", args=[self.progress.pk])) | ||
|
|
||
| # Assert | ||
| self.assertEqual(rv.status_code, 405) |
There was a problem hiding this comment.
You probably don't have to test this, I hope it has been tested by tests for AMYDeleteView.
There was a problem hiding this comment.
I can't find any tests for AMYDeleteView in our code - though I would similarly hope this case is covered by tests for the underlying Django DeleteView. Do you still think it should be removed?
(I copied many of these tests over from TestCRUDViews in test_training_progress.py - this is one of those)
There was a problem hiding this comment.
405 is our custom mechanism for handling GET requests performed on this delete view. If we don't have tests specific for AMYDeleteView then it's okay to keep this test here.
|
@pbanaszkiewicz I made some updates/comments, ready for your re-review. |
| email="[email protected]", | ||
| is_active=True, | ||
| ) | ||
| self.user.set_password("password") |
There was a problem hiding this comment.
Alternatively you can use Person.objects.create_user() (method of PersonManager) which accepts password.
Removes the Get Involved submission form from the training progress view and instead provides create/edit/delete views for the submission. Fixes #2472; fixes #2473.
Allows trainees to view their most recent submission, and edit or delete their submission if it has not yet been reviewed.
These views need some permissions to be set so that people can't edit or delete other TrainingProgresses by changing the ID in the URLs! I'll work on this next - #2493.