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

Use getHome in ChunkLocationProvider.php #40693

Merged
merged 2 commits into from Mar 21, 2023

Conversation

pako81
Copy link
Contributor

@pako81 pako81 commented Mar 21, 2023

Description

Use getHome() in ChunkLocationProvider.php

Related Issue

Motivation and Context

When using the home_folder_naming_rule attribute for defining the home directories for LDAP users (configurable over the LDAP wizard) chunks of users' uploads are wrongly created under the default data directory rather than inside the configured home directory.

This is because currently https://github.com/owncloud/core/blob/v10.12.0/apps/dav/lib/Upload/ChunkLocationProvider.php#L67-L70 is missing a check for such cases. It seems therefore better to rely on the getHome() method for getting the user's home.

How Has This Been Tested?

Manually by setting the home_folder_naming_rule attribute for defining the home directories for LDAP users and observing chunks are now uploaded inside the configured home directory.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised
  • Changelog item

@pako81 pako81 added this to the development milestone Mar 21, 2023
@pako81 pako81 requested a review from jvillafanez March 21, 2023 13:39
@pako81 pako81 self-assigned this Mar 21, 2023
@update-docs
Copy link

update-docs bot commented Mar 21, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@pako81
Copy link
Contributor Author

pako81 commented Mar 21, 2023

@phil-davis I guess this does not require changelog and/or tests as it relates to a change introduced in #40567 ?

@sonarcloud
Copy link

sonarcloud bot commented Mar 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@jnweiger jnweiger merged commit e59215d into master Mar 21, 2023
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the use_gethome_in_chunklocationprovider branch March 21, 2023 22:11
@phil-davis
Copy link
Contributor

@phil-davis I guess this does not require changelog and/or tests as it relates to a change introduced in #40567 ?

That change is already released in 10.12.0. So this is a fix to already-released code. We should put a changelog about the fix, because the behavior in 10.12.0 and the next release (10.12.1 or 10.13.0) will be different.

Also, @pako81 if/when we get a few fixes related to 10.12.0 you can decide if we make a 10.12.1 release to get those "out the door" or if they wait until 10.13.0

@pako81
Copy link
Contributor Author

pako81 commented Mar 22, 2023

That change is already released in 10.12.0. So this is a fix to already-released code. We should put a changelog about the fix, because the behavior in 10.12.0 and the next release (10.12.1 or 10.13.0) will be different.

makes sense - will do.

Also, @pako81 if/when we get a few fixes related to 10.12.0 you can decide if we make a 10.12.1 release to get those "out the door" or if they wait until 10.13.0

Yes, actually we are evaluating together with @jnweiger if this fix would already justify a 10.12.1 release or if we should rather wait. I would probably already go with a 10.12.1.

@pako81 pako81 mentioned this pull request Mar 22, 2023
11 tasks
@grgprarup
Copy link
Contributor

grgprarup commented Mar 24, 2023

@pako81 The WebUI tests are failing in the firewall app after this PR merged. Could you have a look into it?
issue: https://github.com/owncloud/firewall/issues/742
CC @phil-davis

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.

None yet

5 participants