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

Validation for max allowed number of chars in custom groups name #291

Merged
merged 4 commits into from
Jan 28, 2020

Conversation

pako81
Copy link
Contributor

@pako81 pako81 commented Jan 7, 2020

Description

Add validation for maximum allowed number of characters in custom groups names.

Related Issue

Motivation and Context

Since we have already a check in place about the minimum number of chars a custom group name should be (2), it would make sense to also add a check for the maximum length, catch the exception and display a proper warning to the end-user. Currently, no warning is given back to the end-user and an exception is logged in the owncloud.log file.

How Has This Been Tested?

Manually, by trying to create a new / rename an existing custom group having name longer than 64 chars and check that the above-mentioned exception is correctly caught and a proper warning is displayed to the end-user.

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)

@pako81 pako81 added this to the development milestone Jan 7, 2020
@pako81 pako81 self-assigned this Jan 7, 2020
@claassistantio
Copy link

claassistantio commented Jan 7, 2020

CLA assistant check
All committers have signed the CLA.

@pako81 pako81 requested a review from sharidas January 7, 2020 15:49
@pako81 pako81 force-pushed the max-allowed-chars-group-name branch from 27e407b to b85ad48 Compare January 7, 2020 22:07
@codecov
Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #291 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #291   +/-   ##
=========================================
  Coverage     83.34%   83.34%           
  Complexity      316      316           
=========================================
  Files            22       22           
  Lines          1015     1015           
=========================================
  Hits            846      846           
  Misses          169      169

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49bbf33...5a13150. Read the comment docs.

@dpakach
Copy link
Contributor

dpakach commented Jan 8, 2020

@pako81 @phil-davis I have added some acceptance tests for the validation of custom group names length.

@pako81
Copy link
Contributor Author

pako81 commented Jan 8, 2020

@dpakach thanks!

@HanaGemela HanaGemela mentioned this pull request Jan 21, 2020
34 tasks
@pako81
Copy link
Contributor Author

pako81 commented Jan 21, 2020

@dpakach codecov is failing

@phil-davis
Copy link
Contributor

@pako81 Deepak has made acceptance tests, which are not part of code coverage. I guess that code coverage was failing the same before the acceptance tests were added.

@micbar can someone decide if code coverage needs improving? And if so, allocate an appropriate resource (developer?)

@phil-davis
Copy link
Contributor

I rebased just now to get the PR up-to-date and fresh CI

@micbar
Copy link
Contributor

micbar commented Jan 21, 2020

@phil-davis Yes, indeed, we need more coverage.

Currently no dev available. Needs planning.

@phil-davis
Copy link
Contributor

I added unit test cases for long group name validation. It turned out to be easy, and codecov is happy.
@micbar it just needs someone to review/approve/merge this PR.

@micbar micbar requested a review from PVince81 January 21, 2020 19:40
@micbar
Copy link
Contributor

micbar commented Jan 21, 2020

@PVince81 We need some JS know how to review this. Could you? 😄

Copy link

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

Changes seem harmless, also there are similar checks in the changed files. LGTM 👍

@HanaGemela HanaGemela merged commit 8d3947a into master Jan 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the max-allowed-chars-group-name branch January 28, 2020 11:52
/** Group name must be max 64 characters long */
if (\mb_strlen($name, 'UTF-8') > 64) {
throw new ValidationException('The group name should be maximum 64 characters long.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

createGroup() is also called by public function createDirectory() -- should we validate the name there too?

@HanaGemela HanaGemela modified the milestones: development, QA Jan 28, 2020
@HanaGemela HanaGemela mentioned this pull request Jan 28, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants