Skip to content

[10.5.0] Add admin settings checkbox to 'Enable manual file locking on the web UI'#37702

Closed
phil-davis wants to merge 1 commit into
release-10.5.0from
enable_lock_file_action-checkbox-10.5.0
Closed

[10.5.0] Add admin settings checkbox to 'Enable manual file locking on the web UI'#37702
phil-davis wants to merge 1 commit into
release-10.5.0from
enable_lock_file_action-checkbox-10.5.0

Conversation

@phil-davis

@phil-davis phil-davis commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

Description

Adds a checkbox to admin settings "Manual File Locking" to "Enable manual file locking on the web UI"

Checking the checkbox does the same as:

occ config:app:set files enable_lock_file_action --value yes

Unchecking does the same as:

occ config:app:set files enable_lock_file_action --value no

Related Issue

This is a follow-on enhancement from all the file locking additions.

How Has This Been Tested?

Manually check the new checkbox, the browse to the Files page and confirm that the "Lock file" action is available for a file.

Manually uncheck the new checkbox, the browse to the Files page and confirm that the "Lock file" action is not available for a file.

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, see TEMPLATE

@update-docs

update-docs Bot commented Jul 17, 2020

Copy link
Copy Markdown

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.

@phil-davis

Copy link
Copy Markdown
Contributor Author

To decide:

  • does this get added to 10.5.0 or not?
  • if yes, then there is a new string for translation - "Enable manual file locking on the web UI" - how to get that translated into enough languages and get the translations back into 10.5.0

@phil-davis
phil-davis requested a review from micbar July 17, 2020 10:35
@phil-davis phil-davis changed the title Add admin setttings checkbox to 'Enable manual file locking on the we [10.5.0] Add admin setttings checkbox to 'Enable manual file locking on the we Jul 17, 2020
@phil-davis phil-davis changed the title [10.5.0] Add admin setttings checkbox to 'Enable manual file locking on the we [10.5.0] Add admin settings checkbox to 'Enable manual file locking on the web UI' Jul 17, 2020
$tmpl = new Template('settings', 'panels/admin/persistentlocking');
$tmpl->assign('defaultTimeout', $this->config->getAppValue('core', 'lock_timeout_default', LockManager::LOCK_TIMEOUT_DEFAULT));
$tmpl->assign('maximumTimeout', $this->config->getAppValue('core', 'lock_timeout_max', LockManager::LOCK_TIMEOUT_MAX));
$tmpl->assign('manualFileLockOnWebUIEnabled', $this->config->getAppValue('files', 'enable_lock_file_action', 'no'));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is on a core panel outside of the files app, but references a files app setting.
Is that OK?
I don't know what to do when a settings panel needs to have settings from a mix of core and apps.

var minRange = currentInput.attr('min');
var maxRange = currentInput.attr('max');
if (name === 'enable_lock_file_action') {
app = 'files';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This adds code to core settings panel JS that is aware of and manipulates a files app setting.
Is this OK?
How to do it better?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe enable_lock_file_action could be moved from files app to core app? Or?

@codecov

codecov Bot commented Jul 17, 2020

Copy link
Copy Markdown

Codecov Report

Merging #37702 into release-10.5.0 will decrease coverage by 0.00%.
The diff coverage is 33.33%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##             release-10.5.0   #37702      +/-   ##
====================================================
- Coverage             64.74%   64.74%   -0.01%     
  Complexity            19363    19363              
====================================================
  Files                  1281     1281              
  Lines                 75628    75630       +2     
  Branches               1333     1333              
====================================================
+ Hits                  48966    48967       +1     
- Misses                26270    26271       +1     
  Partials                392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.92% <33.33%> (-0.01%) 19363.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
apps/files/lib/App.php 20.00% <0.00%> (ø) 3.00 <0.00> (ø)
...tings/templates/panels/admin/persistentlocking.php 66.66% <0.00%> (-33.34%) 0.00 <0.00> (ø)
settings/Panels/Admin/PersistentLocking.php 100.00% <100.00%> (ø) 4.00 <0.00> (ø)

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 ee3b6a7...7c5517f. Read the comment docs.

@phil-davis

Copy link
Copy Markdown
Contributor Author

What happens about code coverage here? Do I need to add PHP unit tests? If so, where?

@phil-davis
phil-davis force-pushed the enable_lock_file_action-checkbox-10.5.0 branch from 7c5517f to d2d5687 Compare July 21, 2020 03:11
@phil-davis

Copy link
Copy Markdown
Contributor Author

I raised a new PR to master - #37720

@phil-davis phil-davis closed this Jul 22, 2020
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.

1 participant