Skip to content

Conversation

@skshetry
Copy link
Member

@skshetry skshetry commented Nov 23, 2018

Description

The OCC command didn't allow us to set value 0 due to the use of if ($value) checks. Now, This PR modifies it to check for null value only.

Related Issue

Motivation and Context

How Has This Been Tested?

  • Locally

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)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@skshetry skshetry requested a review from icewind1991 November 23, 2018 07:09
@phil-davis phil-davis requested review from DeepDiver1975 and PVince81 and removed request for icewind1991 November 23, 2018 07:28
@phil-davis
Copy link
Contributor

@skshetry are there acceptance tests for this yet?

@skshetry
Copy link
Member Author

@phil-davis No, I don't think we have tested occ commands for any apps that are in core. Here's the broader issue: #33051.

This doesn't seem to have any unit tests also. So, I'm thinking of covering the changed code, and needs to figure that out. I'll write acceptance tests for this also.

@skshetry skshetry force-pushed the fix-files_external-option-filesystemcheckchanges branch from 0c83378 to ae91714 Compare November 23, 2018 09:28
@codecov
Copy link

codecov bot commented Nov 23, 2018

Codecov Report

Merging #33632 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #33632      +/-   ##
============================================
+ Coverage     64.24%    64.3%   +0.05%     
  Complexity    18293    18293              
============================================
  Files          1194     1194              
  Lines         69132    69132              
  Branches       1277     1277              
============================================
+ Hits          44414    44454      +40     
+ Misses        24346    24306      -40     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 52.91% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.62% <100%> (+0.06%) 18293 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Command/Config.php 80% <100%> (+80%) 0 <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 c4e9b32...04a94e6. Read the comment docs.

@skshetry skshetry force-pushed the fix-files_external-option-filesystemcheckchanges branch from ae91714 to 957bd46 Compare November 23, 2018 09:39
@skshetry skshetry force-pushed the fix-files_external-option-filesystemcheckchanges branch from 957bd46 to 04a94e6 Compare November 23, 2018 11:19
@skshetry skshetry changed the title Fix #33630. Fix cannot set 0 as value for filesystem_check_changes through OCC command Fix #33630. Fix cannot set 0 as value on files_external through OCC command Nov 23, 2018
@skshetry skshetry added this to the development milestone Nov 23, 2018
@skshetry skshetry mentioned this pull request Nov 23, 2018
7 tasks
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.

👍

$value = $input->getArgument('value');
if ($value) {

if ($value !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

classic PHP 🤦‍♂️

@PVince81 PVince81 merged commit 573fa9c into master Nov 23, 2018
@PVince81 PVince81 deleted the fix-files_external-option-filesystemcheckchanges branch November 23, 2018 14:36
@PVince81
Copy link
Contributor

@skshetry nice find. Please backport this bugfix to stable10

@skshetry
Copy link
Member Author

Backport for stable10 on #33643

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot set filesystem_check_changes to never through occ command

4 participants