Skip to content

Hide e-mail controls in calendar when e-mail sending is disabled - #4386

Closed
pboguslawski wants to merge 2 commits into
nextcloud:mainfrom
ibpl:main-IB#1121918
Closed

Hide e-mail controls in calendar when e-mail sending is disabled#4386
pboguslawski wants to merge 2 commits into
nextcloud:mainfrom
ibpl:main-IB#1121918

Conversation

@pboguslawski

Copy link
Copy Markdown
Contributor

This mod hides Send email checkbox from attendee context menu if
dav.sendInvitations is disabled. It also replaces misleading status
message about invitations already sent when adding attendee to event.

Author-Change-Id: IB#1121918
Signed-off-by: Pawel Boguslawski pawel.boguslawski@ib.pl

This mod hides `Send email` checkbox from attendee context menu if
`dav.sendInvitations` is disabled. It also replaces misleading status
message about invitations already sent when adding attendee to event.

Author-Change-Id: IB#1121918
Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>
@codecov

codecov Bot commented Jul 19, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.47%. Comparing base (f6ee090) to head (36f93cd).
⚠️ Report is 2885 commits behind head on main.

Files with missing lines Patch % Lines
...rc/components/Editor/Invitees/InviteesListItem.vue 0.00% 2 Missing ⚠️
...rc/components/Editor/AvatarParticipationStatus.vue 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4386      +/-   ##
============================================
+ Coverage     29.42%   29.47%   +0.04%     
  Complexity      330      330              
============================================
  Files           220      220              
  Lines          7702     7706       +4     
  Branches       1019     1017       -2     
============================================
+ Hits           2266     2271       +5     
+ Misses         5436     5435       -1     
Flag Coverage Δ
javascript 20.53% <40.00%> (+0.01%) ⬆️
php 68.89% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tcitworld

Copy link
Copy Markdown
Member

We may want to display a message on top of the attendee list: « Sending e-mail invitations to attendees is disabled by the administrator »

@miaulalala

Copy link
Copy Markdown
Contributor

PHP tests fail :/ also, you can lint the code base by calling composer run cs:fix so the linter check will pass.

Thanks for the PR!

Author-Change-Id: IB#1121918
Fixes: 0cc44d4
Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>
@pboguslawski

Copy link
Copy Markdown
Contributor Author

We may want to display a message on top of the attendee list: « Sending e-mail invitations to attendees is disabled by the administrator »

Different topic; feel free to implement this separately.

PHP tests fail :/ also, you can lint the code base by calling composer run cs:fix so the linter check will pass.

Fixed in 36f93cd. Please verify (not experienced in nextcloud devel).

organizerName: this.organizerDisplayName,
}),
}
return {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you briefly explain why the check for the isViewedByOrganiser isn't needed any more?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you briefly explain why the check for the isViewedByOrganiser isn't needed any more?

Without this fix, when you create an appointment, calendar will display "Invitation sent" just after attendee is added, before any messages are sent/event saved. This is misleading.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes that is true, but the visibility of the email sending status for the organizer only should still be kept in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nextcloud should not lie to user (now it lies that invitation was sent even before event is saved and before sending may take place; more - it also lies about sent invitation if sending invitations is disabled in Nextcloud...). Nextcloud cannot state that invitation was not sent (i.e. when sending invitations is disabled in Nextcloud) because external client (like Thunderbird) could have done it already. Solution proposed here sounds most reasonably for us. Changing it requires more work (i.e. update status after message was actually sent by Nextcloud) and is outside of scope of this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is not what I'm talking about at all, I agree with you on having a correct state on the email sending. I am talking about the VISIBILITY of the element. It should only be visible to the Organizer of the event. I cannot find this limitation in your code as you have removed it. Does your new code cover this visibility issue in a different way or does this revert previous work?

@pboguslawski pboguslawski Jul 29, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does your new code cover this visibility issue in a different way or does this revert previous work?

After this PR organizer sees same message as any other user ("Has not responded to {organizerName}'s invitation yet").

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok then this code change will remove the limitation. I would ask you to please take a look at the old code and add this back in in a way that works with your changes. @st3iny can you also please have a look at this?

@pboguslawski pboguslawski Jul 29, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This PR displays "Has not responded to..." for ALL users so no need for PR to be updated (code from else was simply moved outside of removed if/else).

@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature request labels Jul 31, 2022

@tynvom-nussoc-1waWge tynvom-nussoc-1waWge left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@SebastianKrupinski

Copy link
Copy Markdown
Contributor

Closing this as this is no longer relevant. All function related to email are automatically disabled when no email is setup

@st3iny

st3iny commented Sep 4, 2025

Copy link
Copy Markdown
Member

Yes, but this PR is still relevant. It is about disabling the option to send emails to invitees if disabled on the backend (RSVP attribute of ATTENDEE properties).

@SebastianKrupinski

Copy link
Copy Markdown
Contributor

Had to open a new branch because I could not push the rebase to this one

#8870

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

Labels

3. to review Waiting for reviews enhancement New feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants