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

fix: show edit inputs only for external shares #3714

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

hamza221
Copy link
Contributor

Steps to reproduce:

  1. Create a poll with some options
  2. Go to share tab and share vial mail
  3. Open the poll with that share link
  4. open the settings
  5. seem them turn green
  6. Hit the fields' save arrow at the right
  7. Displayname can only be set for external shares.

Copy link
Collaborator

@dartcafe dartcafe left a comment

Choose a reason for hiding this comment

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

This will cover only 'external shares'.
Should be also allowed for email shares AFAIR, too.

Maybe computed properties (isAllowedChangeDisplayName, isAllowedChangeEmailAddress) are a better style. Not sure if.

@hamza221
Copy link
Contributor Author

@dartcafe the backend is throwing an error if share type is not Share::TYPE_EXTERNAL
https://github.com/nextcloud/polls/blob/master/lib/Service/ShareService.php#L196-L209

That's how I came up with the conclusion that it should only be visible for external shares, I'm not aware of any limitation on why email shares are allowed
If you think we should also allow it for Share::TYPE_EMAIL I can revert the changes and make it into a backend fix

@dartcafe
Copy link
Collaborator

Ah. OK. now I understand your problem. Maybe it is a good idea to first file a bug issue.

So, if I understood you right, the problem is the rejection of changing the name of an email share. This should be possible, which seems to identify a bug at the backend side.

I have to check this, since AFAIR an email share should be converted to an external share after first call via this share. Similar to creating a new share, when entering the poll via public share.

The sharing system seems still to have some flaws, since some changes a few months ago.

@AndyScherzinger
Copy link
Member

So, if I understood you right, the problem is the rejection of changing the name of an email share. This should be possible, which seems to identify a bug at the backend side.

Yes. So basically when you share a poll via an email address and open the poll with that link. When you try to change the mail address or the name you will be always facing an http 403 or 409

2024-09-19 10_19_55-NVIDIA GeForce Overlay

In the screenshot I changes the mail address and the name from the original values from the share link. they turn green while typing, but hitting the send arrow icon you face the errors seen in the browser console

@dartcafe
Copy link
Collaborator

dartcafe commented Sep 20, 2024

Yes, I could reproduce the bug. But I still think, it should be fixed on backend side, since the rejections is the error. Email shares should also be allowed to change name and email address.

I fear the cause is the missing registration (it was removed for these shares by hiding the registration dialog out of convenience) in case of email shares.

And I assume, contact shares are affected as well.

I will get into it deeper over the weekend.

Signed-off-by: dartcafe <[email protected]>
Signed-off-by: dartcafe <[email protected]>
@dartcafe dartcafe added this to the 7.2.4 milestone Sep 21, 2024
…hare

Convert email and contact shares to external shares on first usage
@dartcafe dartcafe self-requested a review September 24, 2024 19:20
Copy link
Collaborator

@dartcafe dartcafe left a comment

Choose a reason for hiding this comment

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

Both changes together do their work.

Accidentally early merge of the follow up PR.

@dartcafe
Copy link
Collaborator

I've tested the changes:

  • E-Mail and contact shares get converted to an external share at first usage
  • These users are again able to change their names and mail addresses

I will merge this, since I want the update to get out quickly.

@dartcafe dartcafe merged commit 0198ce0 into master Sep 24, 2024
21 checks passed
@AndyScherzinger AndyScherzinger deleted the fix/remove-unusuable-inputs branch September 25, 2024 10:59
dartcafe added a commit that referenced this pull request Sep 26, 2024
Signed-off-by: dartcafe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants