Add initial notice #184
Add initial notice #184
Conversation
src/App.vue
Outdated
| .uk-modal-dialog.uk-margin-auto-vertical | ||
| button.uk-modal-close-default(type='button', uk-close) | ||
| .uk-modal-header | ||
| h2.uk-modal-title Important notice |
There was a problem hiding this comment.
translation support is missing
There was a problem hiding this comment.
indeed, and translation support for handlebars is still missing too
There was a problem hiding this comment.
Will be there shortly ;-)
There was a problem hiding this comment.
Translation support exists but has not been used in this early state. Will wait for the final text.
src/App.vue
Outdated
| @@ -7,15 +7,45 @@ | |||
|
|
|||
| main.uk-width-expand | |||
| router-view | |||
|
|
|||
| #scaleout-notice-modal(uk-modal="bg-close : false") | |||
There was a problem hiding this comment.
Please do not use modals. Blank screen on initial market app startup where the user is asked to activate or disable the market app or similar. But no modals.
|
no idea if this is still needed. owncloud/core#29458 will introduce an explicit config flag which allows us to find out if owncloud is clustered or not. Please let me know if you need some api calls (market frontend and market backend) to query for this setting. I could think of adding notification on top of the page which tell the user that installation/uprade is not possible. installing and upgrading will be disallowed by core |
|
... and disable the install and update buttons |
|
owncloud/core#29458 will be in RC, please adjust this PR accordingly. You can still display whatever message, but only if said setting is set. @DeepDiver1975 we could expose the setting over capabilities API ? |
|
Capabilities API results are available by default in the JS array returned by |
|
@felixheidecke could you adapt to @DeepDiver1975 's comment for cluster flag? |
@hurradieweltgehtunter Sure I can, but it will stray from the original idea. The message would pop-up every time you open the market app and say something like: "Since this is a clustered setup, installing and updating apps will not work. Have fun anyway." If that's okay, I'll need an API route telling me about the setup :-) |
|
@felixheidecke instead of an API route what we could do is to put said code into a separate JS file and only load said file from PHP (include it) whenever the mode is set. But I suspect that this approach is not compatible with webpack / vue JS ? |
|
@PVince81 sorry. That won't work in this case :-| |
|
Isn't there a global settings variable or sth else, which is delivered to the frontend and where the cluster flag could be included? |
either we need to add it to the capabilities API, then it will be available directly. else we need to modify the PHP template to inject it through the DOM in a "data-xxx" attribute |
would this be acceptable ? else we'll need another core PR to add the flag to the capabilities |
|
@PVince81 Would this happen somewhere in the template?? |
|
oops, so the template has no DOM elements at all ? |
|
Eeeeexactly :-) |
|
Feels hacky to use HTML as a vehicle to transport infos from PHP to JS :-/ |
don't look into core PHP code then... Ok, looks like the capabilities is the only way. Adding a new endpoint feels like overkill. |
Config delivers license state, internet connection state and cluster mode
|
I've added/modifed an endpoint as discused with @felixheidecke, see 203a1f6 |
lib/Controller/MarketController.php
Outdated
| Http::STATUS_NOT_FOUND | ||
| ); | ||
| $config = [ | ||
| 'canInstall' => ($this->config->getSystemValue('operation.mode', 'single-instance') === 'single-instance'), |
There was a problem hiding this comment.
no - IAppManager::canInstall() is to be called - or better MarketService::canInstall
|
@felixheidecke good point. Looking at the core code for AppManager::canInstall, I see that i also checks writeability of the apps folder. So might need to tweak the message to say "either you're in a clustered env or apps folder is not writeable". |
|
Could we supply a message/string alongside that says something like 'cluster setup' or 'apps folder not writable' or whatever? |
|
We can supply a message, but it will imply redoing the same check in the market app to tweak the message. Or we need to adjust core to either return a reason string instead of boolean for canInstall, or another method to check for writeability of apps folder. Not sure which one is the least ugly. |
|
Can we please keep it simple? |
|
Please use the marketservice::caninstall |
Done |
Is this the new message to be displayed? 😁 @pmaier1 @DeepDiver1975 |
|
Maybe don't display a warning at all, but grey out the install buttons. And close the install button, display a message like "App cannot be installed over the web UI either because this is a clustered setup or the web server has no permissions to write to the apps folder". Now if we consider the separation between OC admin and sysadmin, the OC admin might not need to know about apps folder permissions and a message like "App cannot be installed over the web UI, please check with the system administrator" + link to docs ? |
|
or => "App cannot be installed over the web UI in this setup" |
|
If the API is smart enough, it should already send a |
what would be possible |
|
any travis hates us - please fix - maybe related to #186 ????? |
3b0c24a to
5ec66fe
Compare
|
See #184 (comment) for updated notice. |
|
@felixheidecke some trouble with travis / node ? |
|
Assuming you were happy with the backend code @DeepDiver1975 ? Who reviews the frontend code ? |
That's always a goooood question. @hurradieweltgehtunter and @tboerger know vue. 🤔 |
|
@felixheidecke please retest this properly, not only the new switch but also the license related code. The server backend has changed and there's a key called "licenseKeyAvailable" but I don't see it read anywhere in the UI code. See https://github.com/owncloud/market/pull/184/files#diff-6248f15975b779eff847acf5c338dd48R277 |
Fixes: #179