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.

Sign up for GitHub

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

Jump to bottom

Assign contributor questionnaires per course type #2373

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Assign contributor questionnaires per course type #2373

wants to merge 5 commits into from

Conversation

jooooosef
Copy link
Collaborator

fix #2299

richardebeling
richardebeling previously requested changes Jan 19, 2025
View reviewed changes
@jooooosef
Copy link
Collaborator Author

jooooosef commented Jan 20, 2025 •
edited
Loading

Currently there are two tests doing similiar things to test the questionaire assignment.
That would be TestSemesterQuestionnaireAssignment and TestSemesterAssignView. Should I merge those tests into one or should I at least move those two closer to each other?
(one is in line 907 and one in line 3813) @richardebeling

@niklasmohrin
Copy link
Member

@jooooosef If merging the tests makes your life easier for your testing here, feel free to do it, otherwise consider opening a separate PR - or we can show you how to clean up the commits on your branch next week and make it into two commits here :)

@jooooosef
Copy link
Collaborator Author

@niklasmohrin I'll take the 2 commits option 👷

@jooooosef jooooosef changed the title Assign contributor questionnaires per course type 🚸 Assign contributor questionnaires per course type Jan 27, 2025
@jooooosef jooooosef force-pushed the iss2299-new branch 2 times, most recently from ad2fbb8 to d9a9d31 Compare January 27, 2025 18:36
@jooooosef jooooosef changed the title 🚸 Assign contributor questionnaires per course type Assign contributor questionnaires per course type Jan 27, 2025
Kakadus
Kakadus reviewed Jan 27, 2025
View reviewed changes
@janno42 janno42 self-requested a review February 10, 2025 18:33
@jooooosef
general and contributor questionnaires per course type
284c547 
added the option to specify course types, where specific contributor questionaires should be added to all such courses
@jooooosef
moved tests next to each other
455425e 
one test that tests questionaire assignemnt was 3k lines below the other one
@jooooosef
fixed tests
25c52c0
richardebeling
richardebeling reviewed Feb 10, 2025
View reviewed changes
@richardebeling richardebeling self-requested a review February 10, 2025 22:39
janno42
janno42 approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

Fantastic 🌱

niklasmohrin
niklasmohrin requested changes Feb 24, 2025
View reviewed changes
evap/staff/forms.py Outdated
Comment on lines 930 to 931
self.fields["all-contributors"] = forms.ModelMultipleChoiceField(
label=_("All contributors"), required=False, queryset=contributor_questionnaires
Copy link
Member

Choose a reason for hiding this comment

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

@janno42 In the issue, you said

and for contributors the current field "All contributors" should also be displayed as the last input field

but it's missing and you already approved - do we not need this after all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did implemented it with the last fixup.
If necessary I can remove again.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, @niklasmohrin
Yes, we need that, thanks for adding @jooooosef

richardebeling
richardebeling approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

thanks :)

@jooooosef
fixup! general and contributor questionnaires per course type added t…
d1ddb19 
…he option to specify course types, where specific contributor questionaires should be added to all such courses
@jooooosef
Copy link
Collaborator Author

jooooosef commented Feb 24, 2025 •
edited
Loading

I just realised:
If a person is marked as "Add person without questions" this assign questionaires will overwrite this option and apply the selected questionaires anyways:
image
Is this intended behaviour @janno42 ?

Also: This deletes all previously assigned questionaires and assings the new ones. I think that this will delete all previously assigned questionaires and will only use the newly selected ones should be more clear in the info text. Alternatively there could be a confirm option added which then explains that this will remove the currently assigned questionaires?

@janno42
Copy link
Member

janno42 commented Mar 3, 2025

I just realised: If a person is marked as "Add person without questions" this assign questionaires will overwrite this option and apply the selected questionaires anyways. Is this intended behaviour

Also: This deletes all previously assigned questionaires and assings the new ones. I think that this will delete all previously assigned questionaires and will only use the newly selected ones should be more clear in the info text. Alternatively there could be a confirm option added which then explains that this will remove the currently assigned questionaires?

This is all correct. We could change the info text and add a sentence "This will only change evaluations in preparation. Their questionnaires will be overwritten by this operation."
Adding people without questionnaires is usually done after this bulk assignment, so this is no problem.

janno42
janno42 requested changes Mar 3, 2025
View reviewed changes
evap/staff/templates/staff_semester_questionnaire_assign_form.html
<h4 class="card-title">{% translate 'Contributors' %}</h4>
<fieldset>
{% for field in contributor_fields %}
{% include 'bootstrap_form_field.html' with field=field %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{% include 'bootstrap_form_field.html' with field=field %}
{% include 'bootstrap_form_field.html' with field=field wide=True %}

evap/staff/templates/staff_semester_questionnaire_assign_form.html
<h4 class="card-title">{% translate 'General' %}</h4>
<fieldset>
{% for field in general_fields %}
{% include 'bootstrap_form_field.html' with field=field %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{% include 'bootstrap_form_field.html' with field=field %}
{% include 'bootstrap_form_field.html' with field=field wide=True %}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janno42 janno42 janno42 requested changes

@niklasmohrin niklasmohrin niklasmohrin requested changes

@richardebeling richardebeling richardebeling approved these changes

@Kakadus Kakadus Kakadus approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assign contributor questionnaires per course type
5 participants
@jooooosef @niklasmohrin @janno42 @richardebeling @Kakadus