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

allow multiple comments per participation #1457

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

Conversation

jeriox
Copy link
Contributor

@jeriox jeriox commented Jan 5, 2025

closes #230 closes #1214

ToDo

  • set author
  • add previous comments as readonly to form
  • proper design
  • controls for visibility
  • fix tests
  • automatic comments for state change?

@jeriox jeriox added the [C] feature New feature or request label Jan 5, 2025
@jeriox jeriox force-pushed the participation-comment branch from eae8cbe to a30d7ba Compare March 3, 2025 19:59
@jeriox jeriox requested a review from felixrindt March 3, 2025 20:07
@jeriox jeriox marked this pull request as ready for review March 3, 2025 20:07
@jeriox jeriox force-pushed the participation-comment branch from 44606ba to 16dfe9d Compare March 3, 2025 20:55
@coveralls
Copy link

coveralls commented Mar 3, 2025

Coverage Status

coverage: 83.891% (-0.08%) from 83.971%
when pulling e6b605d on participation-comment
into 9b22c56 on main.

@jeriox
Copy link
Contributor Author

jeriox commented Mar 3, 2025

@felixrindt I played around with the state change stuff a bit.
Finding the appropriate modellogging entries is easy: LogEntry.objects.filter(content_object_id=self.instance.pk, content_type=ContentType.objects.get_for_model(instance.__class__)))
But displaying them in a useful way would require merging them with the comments and sorting them afterwards. That is a lot of hassle and I'm not even sure anymore if it is that helpful. So I think I don't want to do that (at least not as part of this PR)

@jeriox jeriox requested a review from felixrindt March 5, 2025 17:17
Comment on lines +58 to +68
def save(self):
with transaction.atomic():
result = super().save()
if comment := self.cleaned_data["comment"]:
ParticipationComment.objects.create(
participation=result,
text=comment,
authored_by_responsible=self.acting_user,
visibile_for=self.get_comment_visibility(),
)
return result
Copy link
Member

Choose a reason for hiding this comment

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

You can use @transaction.atomic() as a decorator iirc

if not self.errors:
start = cleaned_data["individual_start_time"] or self.shift.start_time
end = cleaned_data["individual_end_time"] or self.shift.end_time
if end < start:
self.add_error("individual_end_time", _("End time must not be before start time."))
return cleaned_data

def save(self):
Copy link
Member

Choose a reason for hiding this comment

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

hmm we can't remove the commit parameter here, as e.g. the named teams Participation Form calls save with the commit parameter. I actually ran into the exception while testing.

<p class="mt-0 mb-0 d-flex">
<span class="pe-1"><b>{{ comment.author }}</b></span>
<span class="flex-grow-1">{{ comment.text }}</span>
<span>{% blocktranslate trimmed with comment.created_at|timesince as time %}{{ time }} ago{% endblocktranslate %}</span>
Copy link
Member

Choose a reason for hiding this comment

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

I tried to translate this to german and it said "vor 5 Tage, 23 Stunde" and I didn't like that. "5 Tage, 23 Stunden her" könnte eher gehen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

klingt halt seeeehr informell

Copy link
Member

Choose a reason for hiding this comment

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

Gefällt mir auch nicht... Wie ist das denn bei dem Meldeschluss gelöst?

{% load i18n %}
<div>
{% for comment in comments %}
<p class="mt-0 mb-0 d-flex">
Copy link
Member

Choose a reason for hiding this comment

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

I think when displaying comments, the need some kind of indicator who can see them. When doing the disposition, I might think a user can see a comment I wrote 5 days ago, even though I might have checked the box to hide it from them :/

{{ form.comment.label_tag }}
<span class="float-end form-check">
<input type="checkbox" id="{{ form.comment_is_internal.auto_id }}" name="{{ form.comment_is_internal.html_name }}" class="form-check-input">
{{ form.comment_is_internal.label_tag }}
Copy link
Member

Choose a reason for hiding this comment

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

This does show a colon at the end, which I think doesn't fit here:
image

Comment on lines +23 to +26
<i class="fas fa-comment"></i>
<div class="d-inline">
<span><b>{{ comment.author }}</b></span><br />
<span>{{ comment.text }}</span>
Copy link
Member

Choose a reason for hiding this comment

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

again, I'm missing some kind of indication who can see what. That might be more of a design issue then a coding challenge. I thought about using font weight and color or words like "to you", "to everyone", "to responsibles" to indicate receivers of the comment/message

Comment on lines +22 to +27
def revert_comments(apps, schema_editor):
ParticipationComment = apps.get_model("core", "ParticipationComment")
db_alias = schema_editor.connection.alias
for comment in ParticipationComment.objects.using(db_alias).all():
comment.participation.comment = comment.text
comment.participation.save()
Copy link
Member

Choose a reason for hiding this comment

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

Bonus points for building the reverse method. Do we need the db_alias here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C] feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal participation comment for planners Reason for rejecting a participation
3 participants