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

Prefer API over direct config access #31607

Merged
merged 2 commits into from
Feb 15, 2019

Conversation

butonic
Copy link
Member

@butonic butonic commented Jun 1, 2018

Direct config access should be avoided if we can use a proper API. Also added a TODO for the missing API to enable encryption.

Found while reviewing #31598

@butonic butonic added this to the maybe some day milestone Jun 1, 2018
@butonic butonic requested a review from sharidas June 1, 2018 08:10
@DeepDiver1975
Copy link
Member

as usual - @butonic tests are failing - please fix

@PVince81
Copy link
Contributor

PVince81 commented Feb 8, 2019

@sharidas please take over

@sharidas sharidas force-pushed the prefer_api_over_direct_config_writing branch from 4a749d9 to aeea45d Compare February 15, 2019 10:10
@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #31607 into master will decrease coverage by 17.41%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #31607       +/-   ##
=============================================
- Coverage     65.11%   47.69%   -17.42%     
=============================================
  Files          1199      109     -1090     
  Lines         69629    10361    -59268     
  Branches       1283     1260       -23     
=============================================
- Hits          45336     4942    -40394     
+ Misses        23919     5050    -18869     
+ Partials        374      369        -5
Flag Coverage Δ Complexity Δ
#javascript 52.39% <ø> (-0.71%) 0 <ø> (ø)
#phpunit 37.31% <ø> (-29.19%) 0 <ø> (-18338)
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Command/Config.php 0% <0%> (-80%) 0% <0%> (ø)
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
core/js/files/fileinfo.js 85% <0%> (-5%) 0% <0%> (ø)
core/js/sharedialogexpirationview.js 72.05% <0%> (-4.51%) 0% <0%> (ø)
apps/files_external/lib/Command/ListCommand.php 67.72% <0%> (-3.79%) 0% <0%> (ø)
apps/files_sharing/js/app.js 70.62% <0%> (-2.75%) 0% <0%> (ø)
apps/files_external/js/settings.js 49.29% <0%> (-2.55%) 0% <0%> (ø)
core/js/sharedialogview.js 76.41% <0%> (-2.38%) 0% <0%> (ø)
core/js/sharedialogmailview.js 60.39% <0%> (-1.99%) 0% <0%> (ø)
core/js/files/client.js 80.99% <0%> (-1.86%) 0% <0%> (ø)
... and 1108 more

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 c7bfdea...aeea45d. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #31607 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #31607      +/-   ##
============================================
+ Coverage     65.11%   65.11%   +<.01%     
  Complexity    18338    18338              
============================================
  Files          1199     1199              
  Lines         69629    69630       +1     
  Branches       1283     1283              
============================================
+ Hits          45336    45337       +1     
  Misses        23919    23919              
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 53.1% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.5% <100%> (ø) 18338 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/Command/Encryption/Enable.php 100% <100%> (ø) 7 <0> (ø) ⬇️
...eratedfilesharing/lib/Controller/OcmController.php 66.26% <0%> (+0.2%) 30% <0%> (ø) ⬇️

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 c7bfdea...64cb9d3. Read the comment docs.

@sharidas
Copy link
Contributor

Updated the PR with the unit test.
Regarding TODO mentioned at https://github.com/owncloud/core/pull/31607/files#diff-8723f509ea809a9e16fb75a2945bf960R59, apart from the encryption command(s), I found only one place in core https://github.com/owncloud/core/blob/master/settings/js/admin.js#L29

Update unit test for encryption enable command

Signed-off-by: Sujith H <[email protected]>
@sharidas sharidas force-pushed the prefer_api_over_direct_config_writing branch from aeea45d to 64cb9d3 Compare February 15, 2019 10:14
Copy link
Contributor

@sharidas sharidas left a comment

Choose a reason for hiding this comment

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

Apart from the minor comment regarding TODO, LGTM 👍

@PVince81 PVince81 merged commit f988491 into master Feb 15, 2019
@PVince81 PVince81 deleted the prefer_api_over_direct_config_writing branch February 15, 2019 11:15
@PVince81
Copy link
Contributor

@sharidas please backport

@sharidas
Copy link
Contributor

Backport PR : #34504

@sharidas
Copy link
Contributor

Need to merge #34505.
Reason for this change : #34505 (comment). I missed the dead code part in the editor. No backport required for #34505. Because it is already covered in the backport PR #34504

@lock lock bot locked as resolved and limited conversation to collaborators Feb 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants