Skip to content

Add passwordEnabled field to postHookData #28822

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.

Sign up for GitHub

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

Jump to bottom
Merged
PVince81 merged 1 commit into from
Aug 29, 2017
Merged

Add passwordEnabled field to postHookData #28822

PVince81 merged 1 commit into from
Aug 29, 2017

Conversation

@sharidas
Copy link
Contributor

@sharidas sharidas commented Aug 28, 2017

Add passwordDisabled field to postHookData

Signed-off-by: Sujith H [email protected]

Description

Adding additional parameter passwordDisabled to the postHookData. This data would help us to determine if password is enabled for the share or not while creating the share.

Related Issue

Motivation and Context

This would help us know if the share was created with password or not.

How Has This Been Tested?

  • Create public folder share with password - the logs should help us understand that password was enabled.
  • Create public folder share without password - the logs should help us understand that password wasn't enabled.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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 read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@sharidas sharidas requested a review from PVince81 August 28, 2017 15:37
@sharidas sharidas added this to the QA milestone Aug 28, 2017
@sharidas sharidas self-assigned this Aug 28, 2017
PVince81
PVince81 reviewed Aug 28, 2017
View reviewed changes
lib/private/Share20/Manager.php Outdated
Copy link
Contributor

@PVince81 PVince81 Aug 28, 2017

Choose a reason for hiding this comment

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

wouldn't this pass the password string instead of a boolean ?

call it "passwordEnabled" and set it to true if a password is set instead

@sharidas sharidas force-pushed the update-password-postshare-hook branch from 23828c5 to 56d5241 Compare August 28, 2017 15:49
@sharidas
Copy link
Contributor Author

sharidas commented Aug 28, 2017

Updated the PR to return true/false if password is enabed/disabled.

@sharidas sharidas force-pushed the update-password-postshare-hook branch from 56d5241 to f6a814a Compare August 28, 2017 15:51
PVince81
PVince81 reviewed Aug 28, 2017
View reviewed changes
lib/private/Share20/Manager.php Outdated
Copy link
Contributor

@PVince81 PVince81 Aug 28, 2017

Choose a reason for hiding this comment

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

is it always null or can it sometimes be empty string after clearing ? please check

Copy link
Contributor Author

@sharidas sharidas Aug 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

if I replace is_null with empty then also it's working. I have verified it by creating public link with password and creating public link without password.
When the password field is cleared after set the hook related to that is https://github.com/owncloud/core/blob/master/lib/private/Share20/Manager.php#L733 post_update_password.

Copy link
Contributor

@phil-davis phil-davis Aug 29, 2017

Choose a reason for hiding this comment

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

The problem with empty() will be when someone sets the share password to "0". PHP is very annoying.

@sharidas sharidas force-pushed the update-password-postshare-hook branch 4 times, most recently from 97b7e07 to b34a5d3 Compare August 28, 2017 17:13
@sharidas sharidas changed the title Add passwordDisabled field to postHookData Add passwordEnabled field to postHookData Aug 28, 2017
PVince81
PVince81 reviewed Aug 29, 2017
View reviewed changes
lib/private/Share20/Manager.php Outdated
Copy link
Contributor

@PVince81 PVince81 Aug 29, 2017

Choose a reason for hiding this comment

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

no need for true : false, it is implicitly boolean when you have a condition

PVince81
PVince81 approved these changes Aug 29, 2017
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 otherwise

@SergioBertolinSG SergioBertolinSG self-requested a review August 29, 2017 07:39
SergioBertolinSG
SergioBertolinSG approved these changes Aug 29, 2017
View reviewed changes
Copy link
Contributor

@SergioBertolinSG SergioBertolinSG left a comment

Choose a reason for hiding this comment

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

Fixes the issue.

@sharidas sharidas force-pushed the update-password-postshare-hook branch 2 times, most recently from 5cec8fa to 0bd8977 Compare August 29, 2017 07:43
@sharidas
Add passwordEnabled field to postHookData
7b1c8c8 
Add passwordEnabled field to postHookData
A boolean value which help us know if password
is provided during share creation or not.

Signed-off-by: Sujith H <[email protected]>
@sharidas sharidas force-pushed the update-password-postshare-hook branch from 0bd8977 to 7b1c8c8 Compare August 29, 2017 08:04
@PVince81 PVince81 merged commit 64f521a into master Aug 29, 2017
@PVince81 PVince81 deleted the update-password-postshare-hook branch August 29, 2017 13:21
@felixboehm felixboehm modified the milestones: QA, development Sep 4, 2017
@sharidas
Copy link
Contributor Author

sharidas commented Nov 3, 2017

Backport to stable10 of this PR: #28827

@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@phil-davis phil-davis phil-davis left review comments

+2 more reviewers

@PVince81 PVince81 PVince81 approved these changes

@SergioBertolinSG SergioBertolinSG SergioBertolinSG approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sharidas sharidas

Labels

3 - To Review status/STALE

Projects

None yet

Milestone

10.0.3

Development

Successfully merging this pull request may close these issues.

8 participants

@sharidas @PVince81 @phil-davis @SergioBertolinSG @DeepDiver1975 @felixboehm @ownclouders