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

Notify by email on group share can be done multiple times #35089

Closed
dpakach opened this issue Apr 24, 2019 · 11 comments · Fixed by #35136
Closed

Notify by email on group share can be done multiple times #35089

dpakach opened this issue Apr 24, 2019 · 11 comments · Fixed by #35136
Assignees
Labels
p2-high Escalation, on top of current planning, release blocker regression Type:Bug
Milestone

Comments

@dpakach
Copy link
Contributor

dpakach commented Apr 24, 2019

Steps to reproduce

  1. Log in as admin, enable share notifications by email
  2. create a group with multiple users in it
  3. Share a file with the group and click notify by email button
  4. reload the page.

Expected behaviour

according to #33180 the notify by email button should disappear after sending the email once.

Actual behaviour

The notify by email button reappears and the sharer can send email to all users on the group again.

@PVince81
Copy link
Contributor

can you check if this happens on 10.1.1 also ? just to know whether it's a regression

@patrickjahns patrickjahns self-assigned this Apr 26, 2019
@patrickjahns
Copy link
Contributor

IF not a regression - we'll talk with product management about desired behaviour and document it via an acceptance test

@dpakach
Copy link
Contributor Author

dpakach commented Apr 30, 2019

@PVince81 @patrickjahns The problem doesnot exist in 10.1.1, Seems like it was introduced by #33180

@PVince81 PVince81 added regression p2-high Escalation, on top of current planning, release blocker labels Apr 30, 2019
@PVince81
Copy link
Contributor

ok, it's a regression then. thanks

@PVince81
Copy link
Contributor

#33180 is the backport of @VicDeo's code cleanup.

@VicDeo do you have any clue whether this would be an easy fix or should we revert ?

@PVince81
Copy link
Contributor

PVince81 commented May 2, 2019

@dpakach can we cover this with a UI test maybe ?

the cause of the issue was related to some code cleanup of old APIs, now we'll revert partly. there is a risk that a future backport of another related cleanup would bring this issue back in some form, so it might be worth covering this with acceptance tests then

@PVince81
Copy link
Contributor

PVince81 commented May 2, 2019

this also means that the feature is broken currently on master

@patrickjahns
Copy link
Contributor

there is a risk that a future backport of another related cleanup would bring this issue back in some form, so it might be worth covering this with acceptance tests then

@PVince81 @phil-davis - do we have a ticket to track acceptance tests for this issue yet?

@phil-davis
Copy link
Contributor

@dpakach are there tests anywhere for this? Or an issue for making tests?

@dpakach
Copy link
Contributor Author

dpakach commented May 10, 2019

@phil-davis There are no tests or issues for this scenario.
I will make a issue for this now.

@dpakach
Copy link
Contributor Author

dpakach commented May 10, 2019

Issue raised at #35195

@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-high Escalation, on top of current planning, release blocker regression Type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants