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

Add field orga_notes to submissions for internal notes by the organisers #532

Merged
merged 2 commits into from Mar 4, 2019

Conversation

@Nakaner
Copy link
Contributor

@Nakaner Nakaner commented Nov 12, 2018

As a Django noob, I am happy to be able to submit my first pull request.

I would like to share notes about the submission, e.g. things I discussed with the submitter, with other reviewers/organisers. I don't or can't write everything into the notes field because it is visible to the submitter.

Screenshots (if appropriate):

notes-orga

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed. – unable to run them, see #531
  • My change is listed in the CHANGELOG.rst if appropriate.
Copy link
Member

@rixx rixx left a comment

This field should also be present in the API, if an organiser requests it, so please add it to the appropriate serialiser, and the API documentation.

@@ -97,6 +97,7 @@
{% if form.abstract %}{% bootstrap_field form.abstract layout='event' %}{% endif %}
{% if form.description %}{% bootstrap_field form.description layout='event' %}{% endif %}
{% if form.notes %}{% bootstrap_field form.notes layout='event' %}{% endif %}
{% if form.orga_notes %}{% bootstrap_field form.orga_notes layout='event' %}{% endif %}
Copy link
Member

@rixx rixx Nov 12, 2018

Why wouldn't the field be present? Do you want it to be hidden towards reviewers?

@@ -124,6 +124,14 @@ class Submission(LogMixin, models.Model):
notes = models.TextField(
null=True, blank=True, verbose_name=_('Notes for the organiser')
)
orga_notes = models.TextField(
Copy link
Member

@rixx rixx Nov 12, 2018

Please call the field internal_notes instead.

Copy link
Contributor Author

@Nakaner Nakaner Nov 26, 2018

The notes field is currently not exposed by the API as well. Shall I add it?

@rixx
Copy link
Member

@rixx rixx commented Nov 23, 2018

Just a short ping: are you still interested in pursuing this PR?

@Nakaner Nakaner force-pushed the orga_notes branch 2 times, most recently from 110b75d to 72967b5 Nov 26, 2018
Copy link
Member

@rixx rixx left a comment

I'd expect the internal notes field to be present in the API for organisers, too, but if you don't want to touch the API on your first PR, I'll be happy to add that part myself. Looking good otherwise apart from the small wording changes!

src/pretalx/submission/models/submission.py Outdated Show resolved Hide resolved
src/pretalx/submission/models/submission.py Outdated Show resolved Hide resolved
@Nakaner
Copy link
Contributor Author

@Nakaner Nakaner commented Mar 4, 2019

I rebased on current master and merged migrations (manage.py makemigrations --merge).

I decided against adding the internal_notes field to the API output in this pull request because it is not just adding a single line. Currently, the /submissions endpoint returns public fields only. Therefore, the view should be made aware about the permissions of the user.

Copy link
Member

@rixx rixx left a comment

I decided against adding the internal_notes field to the API output in this pull request because it is not just adding a single line. Currently, the /submissions endpoint returns public fields only. Therefore, the view should be made aware about the permissions of the user.

Well, the submissions endpoint already returns different data depending on if the user has organiser access, but as I said: it's alright if you don't want to touch the API right now.

from django.db import migrations


class Migration(migrations.Migration):
Copy link
Member

@rixx rixx Mar 4, 2019

Just a heads-up, I'll probably remove your migrations before the merge, since there's no real reason to keep merge migrations.

@rixx rixx dismissed their stale review Mar 4, 2019

I'll address the remaining issues myself.

@rixx rixx force-pushed the orga_notes branch 2 times, most recently from f88050a to 7464554 Mar 4, 2019
@codecov
Copy link

@codecov codecov bot commented Mar 4, 2019

Codecov Report

Merging #532 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   88.75%   88.75%   +<.01%     
==========================================
  Files         137      137              
  Lines        7870     7871       +1     
  Branches      961      961              
==========================================
+ Hits         6985     6986       +1     
  Misses        629      629              
  Partials      256      256
Impacted Files Coverage Δ
src/pretalx/orga/forms/submission.py 82.75% <ø> (ø) ⬆️
src/pretalx/submission/models/submission.py 95.09% <100%> (+0.01%) ⬆️

@rixx rixx merged commit 5abed45 into pretalx:master Mar 4, 2019
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants