Skip to content
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

Instructor training workflow #871

Merged
merged 20 commits into from Aug 4, 2016

Conversation

chrismedrela
Copy link
Contributor

@pbanaszkiewicz , here is the schema proposal, it turns out we need only two models. Please review. And don't merge. :)

@chrismedrela
Copy link
Contributor Author

chrismedrela commented Jul 12, 2016

Ready for review.

Issues:

  1. I've left some questions in the code (see FIXME comments).
  2. Handling of multiple forms on one webpage. For example, have a look at trainees view. There are two functionalities: bulk discarding and bulk add progresses. They have to live in the same <form> tag, because they share checkboxes that let you select trainees. This results in a lot of ugly workarounds in the code.
  3. Better way to display discarded progresses. They're striked and there is cross icon now. The strike is not visible and the cross can be misinterpreted as delete button.
  4. I've put everything into one commit, because I don't want to have history going like this: "fix A", "implement B", "introduce C feature", "bunch of changes to A, B and C", "fix bug in A and C", "another bunch of changes to A, B and C". So I'd like to split it into smaller commits after final review, just before merging. Look at this commit message to see how the messages of smaller commits will look like.
  5. Let me know if there is enough comments and docstrings.

@gvwilson
Copy link
Contributor

@chrismedrela can you please re-base? @pbanaszkiewicz can you please do a technical review (esp. look for the FIXME lines)?

@@ -207,6 +211,139 @@ def get_order_by(self, order_value):
return super().get_order_by(order_value)


def filter_all_persons(queryset, all_persons):
if all_persons:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a docstring -- I understood the purpose of this function after reading all_persons label.

@chrismedrela
Copy link
Contributor Author

@pbanaszkiewicz, I've implemented changes that you proposed and rebased against the newest develop. The parent commit ID is b65107d

I suggest you to provide split PRs for future reviews

Do you mean multiple PRs or one PRs with multiple commits? The former is not possible, because a lot of changes depends on other changes.

Please ignore import_training_progress.py file for a moment, I'm working on it.

@pbanaszkiewicz
Copy link
Contributor

I suggest you to provide split PRs for future reviews

Do you mean multiple PRs or one PRs with multiple commits? The former is not possible, because a lot of changes depends on other changes.

I mean turn single-commit-PRs into multiple-commit-PRs. It was whole lot more convenient for me to read through commits in #905 than reading through this never ending stream of code lines.

Anyway, this PR looks good enough for me, please merge when you think you're ready.

From test_landing_page.py to test_trainee_dashboard.py.
Add '#sponsors' to the redirect url, so that you're redirected to
"Sponsors" tab.
- Delete unnecessary spaces at the beginning and at the end of docstrings:

    """ Dostring. """ --> """Docstring."""

- Remove unused imports.

- Optimize imports (that is, sort them).
so they look like a link.
GET requests to views inheriting from DeleteViewContext returns 405
(method not allowed) now.
Use this mixin with form views, so that after successful form
submission, the user is redirected to whatever GET['next'] is set to, if
it is not missing.
and get rid of duplicated lines in subclasses.
Changes in form helpers:

- Replace BootstrapHelperWithAdd with BootstrapHelper(submit_label='Add')

- Replace BootstrapHelperWiderLabels with BootstrapHelper(wider_labels=True)

- Replace BootstrapHelperGet with BootstrapHelper(use_get_method=True)

- Add BootstrapHelper.duplicate_buttons_on_top flag

  When this flag is set to True, submit buttons are displayed on both
  top and bottom of a form. The default value is False.

- Add other arguments to BootstrapHelper.__init__

Additionally, we now use form.helper instead of form_helper.

So far, FormHelper and Forms were created in a view independently, put
into a context and passed to a template where they were connected:

    {% crispy form form_helper %}

Now the helper is set directly on the forms. You don't have to think
about the helper in a view or in a template (unless you want it to be
request-dependent, i.e. when you want form_action to depend on the
request).

One more change: duplicated code (sidebar) in templates was extracted
and moved to base template.
On every page, you can use new functionalities now:

- Set `select-all-checkbox` attribute on checkbox to let user
  select/unselect all checkboxes with `respond-to-select-all-checkbox`
  attribute in the same form.

- Bootstrap popovers and tooltips work on each page by default.

- Set `bulk-email-on-click` attribute on a button to email group
  of selected people.

Other changes:

- Remove empty extrajs blocks in templates.

- Remove JS chunks that are unnecessary now.
- introduce new models: TrainingProgress and TrainingRequirement

- add `TrainingRequest.state` and `TrainingRequest.person` fields

- add `TrainingRequirement` to django-admin site

- put everything in one migration
@chrismedrela
Copy link
Contributor Author

chrismedrela commented Aug 4, 2016

@gvwilson, I need URL to Etherpad where you can register for Demo Session. This URL will be displayed in trainee's dashboard. I'll merge this branch today (a hundred per cent sure).

@pbanaszkiewicz I hope we'll work on smaller PRs now. This is more convenient for both the contributor and the reviewer.

@gvwilson
Copy link
Contributor

gvwilson commented Aug 4, 2016

http://pad.software-carpentry.org/teaching-demos

On 2016-08-04 8:03 AM, Christopher Medrela wrote:

@gvwilson https://github.com/gvwilson, I need URL to Etherpad where
you can register for Demo Session. This URL will be displayed in
trainee's dashboard.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#871 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA3ozjH8H0Or5bKI34KNp_MVlYa6rHTUks5qcdUYgaJpZM4JDP0t.

Dr Greg Wilson
Director of Instructor Training
Software Carpentry Foundation

- display progress of becoming SWC/DC Instructor

- let the trainee send SWC or DC homework using SendHomeworkForm
- add bulk operations:

    - discard selected requests (BulkChangeTrainingRequestForm)

    - unmatch selected people from trainings
      (BulkChangeTrainingRequestForm again)

    - match selected people to chosen training
      (BulkMatchTrainingRequestForm)

    - mail selected people

- added filtering sidebar (TrainingRequestFilter)

- fix carpentries#926 -- download all training requests as CSV file
- display new fields (state and matched person)

- add form to match request with selected person or with a new one
  (AcceptTrainingRequestForm); this form suggest matching Person
- add filtering sidebar (TraineeFilter, TTTEventLookup)

- add bulk operations:

    - add training progress (BulkAddTrainingProgressForm)

    - discard training progress (BulkDiscardProgressesForm)
@chrismedrela chrismedrela merged commit beafe92 into carpentries:develop Aug 4, 2016
@gvwilson
Copy link
Contributor

gvwilson commented Aug 4, 2016

Excellent work - thank you.

@pbanaszkiewicz
Copy link
Contributor

Thanks @chrismedrela!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants