Add module "Not available" option in spaces or for users

[marc-farre]

Is your feature request related to a problem? Please describe.

Example with the wiki module.
You want to allow activating the module in spaces but not for users.

Describe the solution you'd like

[marc-farre]

@luke- if you agree with this idea, I can submit a PR. Humhub 1.15 or 1.16?

[luke-]

Yes, an "Not available" option would be good.

But I am not sure if the "Defaults" modal is the right place here.

There is also a similar feature here: #3415
However, we need to revise this PR again.

[marc-farre]

Yes, I agree the "set as default" is not appropriate for making the module not available on a container types.

However, it might add complexity to add another page for that.

Perhaps we could rename the "Set as default module" modal box title with something like "Activation in spaces and for user" and then rename the "Deactivated" and "Activated" options with "Deactivated by default" and "Activated by default"?

[luke-]

For now, I think it's fine to add the "Not available" option to the "Default" modal.

If we add a way to restrict modules by container (e.g. Module Wiki for Space A only), we can move this new option to this dialog.

[marc-farre]

OK, fine. So I'll add a check of what's chosen into ContentContainerModule::hasContentContainerType().

How should I deal with already activated modules in containers?

Should I:

1. just hide them and keep the data if available again later
2. or should I disable them and execute the ContentContainerModule::disableContentContainer() in which case I need to add warning when saving the form if "Not available" is selected?

[marc-farre]

Option 1 (just hide) is a little tricky to do because I cannot filter easily content related to "not available" modules in ContentContainerStreamFilter::apply(), unless I add a column in the content table to indicate the module ID of the content.

If I had this module ID, I could filter this way:

        $enabledModuleIds = array_map(static function ($contentContainerModule) {
            return $contentContainerModule;
        }, $this->container->moduleManager->getAvailable());
        $this->query->andWhere(['content.module_id' => $enabledModuleIds]);

@luke- What do you think about this idea? Or should I do option 2 (disable the module and delete data)?

[luke-]

@yurabakhtin Can you please help @funkycram here?

[marc-farre]

@yurabakhtin I've started with this: 7e86237
Thanks!

[yurabakhtin]

@yurabakhtin I've started with this: 7e86237 Thanks!

@funkycram I think instead of the new code in the function hasContentContainerType you need this code:

if (ContentContainerModuleManager::getDefaultState($containerClass, $this->id) === ContentContainerModuleState::STATE_NOT_AVAILABLE) {
    return false;
}

However I am not sure we need to modify the function because when you update the form "Set as default" with new option "Not available" then it will be impossible to update it again because it will looks like this module is completely disabled from module side:

I mean it will looks like the module has the code:

public function getContentContainerTypes()
{
    return [
        Space::class
    ];
}

but really it has:

public function getContentContainerTypes()
{
    return [
        Space::class,
        User::class,
    ];
}

So we need to find another place where to disable the module with new "Not available" option.

[yurabakhtin]

@funkycram I think this condition https://github.com/humhub/humhub/blob/develop/protected/humhub/modules/content/components/ContentContainerModuleManager.php#L166-L167:

if ($module instanceof ContentContainerModule && $module->isActivated &&
    $module->hasContentContainerType(get_class($this->contentContainer))) {

should be added with new:

ContentContainerModuleManager::getDefaultState(get_class($this->contentContainer), $module->id) !== ContentContainerModuleState::STATE_NOT_AVAILABLE

It would be better to wrap the big condition to new separate method like this:

public function isAvailableModule($module): bool
{
    return $module instanceof ContentContainerModule &&
        $module->isActivated &&
        $module->hasContentContainerType(get_class($this->contentContainer)) &&
        self::getDefaultState(get_class($this->contentContainer), $module->id) !== ContentContainerModuleState::STATE_NOT_AVAILABLE;
}

so the conditions will be more readable:

if ($this->isAvailableModule($module)) {
    $this->_available[$module->id] = $module;
}

Please let me know if you need more help from me or if I missed something,
Thanks.

[marc-farre]

Thanks very much @yurabakhtin . That's fine and avoids a DB request.

How about #6486 (comment) about "How should I deal with already activated modules in containers?", the 2 options and the next comment? Thanks!

[yurabakhtin]

How about #6486 (comment) about "How should I deal with already activated modules in containers?", the 2 options and the next comment? Thanks!

@funkycram

How should I deal with already activated modules in containers?

Should I:

1. just hide them and keep the data if available again later
2. or should I disable them and execute the ContentContainerModule::disableContentContainer() in which case I need to add warning when saving the form if "Not available" is selected?

My solution above only hides the already activated modules in containers.
I think it would be better to keep the data for all containers where the module was activated, otherwise many data will be lost.
If admin really wants to delete all data of the module with new option "Not available" then he should disable the module completely either per User/Space or completely in system.
So I prefer 1st solution.

Sure we could implement an additional confirmation with question like "Do you want to delete all module data for all Users/Spaces?" like we ask before disabling, or this option can be implemented as checkbox under the new option with text like "[ ] Delete module data for all Users/Space":

But I am not sure this cleanup option will be popular, or am I wrong?

[marc-farre]

Yes, I agree with you, option 1 is better.
So what do you think about #6486 (comment) ?

Because your solution hides the module, but not the content in the stream. Thanks!

[yurabakhtin]

@funkycram

Yes, I agree with you, option 1 is better. So what do you think about #6486 (comment) ?

Because your solution hides the module, but not the content in the stream. Thanks!

Yes, you are right, it is a bit more work to filter all content of the "not available" modules instead of just remove contents as we do on disabling.

Sure easier to filter content with new column module_id, but I am not sure we should add this new one.
We could try to filter contents with new filter:

<?php
/**
 * @link https://www.humhub.org/
 * @copyright Copyright (c) HumHub GmbH & Co. KG
 * @license https://www.humhub.com/licences
 */

namespace humhub\modules\stream\models\filters;

use humhub\models\Setting;
use humhub\modules\content\components\ContentContainerModule;
use humhub\modules\content\models\ContentContainerModuleState;
use humhub\modules\space\models\Space;
use humhub\modules\user\models\User;
use ReflectionClass;
use Yii;

class ModuleStreamFilter extends StreamQueryFilter
{
    public function apply()
    {
        $this->excludeContainersWithNotAvailableModule(Space::class);
        if (!$this->streamQuery->container instanceof Space) {
            $this->excludeContainersWithNotAvailableModule(User::class);
        }
    }

    private function excludeContainersWithNotAvailableModule(string $containerClass)
    {
        $reflectClass = new ReflectionClass($containerClass);

        // Find modules with "Not Available" option per User/Space
        $moduleIds = Setting::find()
            ->select('module_id')
            ->where(['name' => 'moduleManager.defaultState.' . $reflectClass->getShortName()])
            ->andWhere(['value' => ContentContainerModuleState::STATE_NOT_AVAILABLE])
            ->column();

        if (empty($moduleIds)) {
            return;
        }

        $excludedContentClasses = [];
        foreach ($moduleIds as $moduleId) {
            $module = Yii::$app->getModule($moduleId);
            if ($module instanceof ContentContainerModule) {
                $excludedContentClasses = array_merge($excludedContentClasses, $module->getContentClasses());
            }
        }

        if (empty($excludedContentClasses)) {
            return;
        }

        $this->query->andWhere(['OR',
            ['!=', 'contentcontainer.class', $containerClass],
            ['AND', ['contentcontainer.class' => $containerClass], ['NOT IN', 'content.object_model', $excludedContentClasses]]
        ]);
    }
}

Could you please test it?
Please note to enable this filter just add it to StreamQuery::$filterHandlers = [... , ModuleStreamFilter::class] - https://github.com/humhub/humhub/blob/master/protected/humhub/modules/stream/models/StreamQuery.php#L143

[marc-farre]

Thanks very much @yurabakhtin , it works perfectly!

I've changed slightly the ModuleStreamFilter::apply() method, don't hesitate to change it back to your code if you think it's better.

@luke- PR #6499

[yurabakhtin]

I've changed slightly the ModuleStreamFilter::apply() method, don't hesitate to change it back to your code if you think it's better.

@funkycram Thank you for the PR, I have reviewed it and described why we need the changes, please see my remarks in the PR.

[marc-farre]

@yurabakhtin Thanks very much for the explanation. Now I understand much better how it works. I've applied your solution in commit 05cbb72 and I tested all combinaisons (in space, user and dashboard), it works fine in all cases!

[marc-farre]

@luke- If think it's now okay to merge PR #6499 into develop. Thanks!

[luke-]

@funkycram I would prefer to merge this into v1.16. Is the PR urgent?

[yurabakhtin]

@funkycram @luke- Before merging I would like to fix the issue from here #6499 (comment) at least for the "Files" module.
We should investigate why the module doesn't have the defined classes here humhub\modules\cfiles\Module->getContentClasses(), maybe it was skipped by mistake.

---

Also I think probably we should exclude the blocked classes from the stream filter "Content Type", at least for Space wall stream, but not sure for User wall stream, because there it is not easy to find all spaces where user posted.

[marc-farre]

@luke- yes, let's wait for Humhub 1.16 (I've updated the CHANGELOG-DEV).

@yurabakhtin sorry, I wasn't notified about your review (it's not the first time I'm not notified and I also remember you wasn't notified once about one of my review mentioning you). So I reply to your review here.

@funkycram @luke- I think we should be sure that all modules extended from ContentContainerModule properly define the classes in the method getContentClasses(), because during tests I find at least one module is "Files" https://github.com/humhub/cfiles/blob/master/Module.php which doesn't there records like humhub\modules\cfiles\models\File::class.

I didn't investigate deeply why it was missed there, probably some reason exists.
But if we set the module "Files" to new option "Not available" then the File records will not be hidden from a wall stream as expected, because the records will not be excluded because of humhub\modules\cfiles\Module->getContentClasses() returns empty array now.

Thanks for your investigation.
It seams these modules are also missing getContentClasses():

• External Calendar
• Gallery

Perhaps we could update these 3 modules (Files, External Calendar and Gallery) so they are ready when this "Not available" option arrives in Humhub 1.16?

Also I think probably we should exclude the blocked classes from the stream filter "Content Type", at least for Space wall stream, but not sure for User wall stream, because there it is not easy to find all spaces where user posted.

@yurabakhtin yes sure. What do your think about this commit (tested, it works)?
5465457

[yurabakhtin]

@funkycram @luke-

Thanks for your investigation. It seams these modules are also missing getContentClasses():

• External Calendar
• Gallery

Perhaps we could update these 3 modules (Files, External Calendar and Gallery) so they are ready when this "Not available" option arrives in Humhub 1.16?

I agree we should update the missed getContentClasses() for the modules.
I have tested and don't find any issues after adding the method for the modules.
Probably it was not added before because these modules don't need to a possibility to add a Content Record via wall stream create form or tab as we have for Post, Poll, Wiki and etc. Please note, if we will add the method getContentClasses() it will not display the wall stream create form/tab, and it is ok, so we can add the method without issue I think.

---

Also I think probably we should exclude the blocked classes from the stream filter "Content Type", at least for Space wall stream, but not sure for User wall stream, because there it is not easy to find all spaces where user posted.

@yurabakhtin yes sure. What do your think about this commit (tested, it works)? 5465457

I have tested it works well, thank you!
Only this line 5465457#diff-9153b24b891cb408e44c5f5b71a1e7a96ae1c9ff684e40881df98b7bf5052294R87 can be deleted, because it is not used below.
Please note I have merged develop into your branch enh/6486-add-module-not-available-option-for-containers because the form "Set as Default" was fixed in the develop branch.

[marc-farre]

Thanks @yurabakhtin Yes, this was a forgotten line. Removed in commit b9e7d34

[marc-farre]

@yurabakhtin we also have the Share Between module missing getContentClasses(). So the module list missing this class is:

• External Calendar
• Gallery
• Files
• Share Between

[yurabakhtin]

@luke- @funkycram I find the method ContentContainerModule::getContentClasses() was added here https://github.com/humhub/humhub/pull/5827/files#diff-3634c090cf95aea01f8986dca83fa4721c2f44685893c63d683de1550ffc02f2R227 since v1.13. The method was implemented in order to initialize tabs and the create forms on the wall stream.
As I wrote here #6486 (comment) we can fill the methods for the modules where it was missed without side issues because the tab and create form will be appeared on the wall stream only if Wall Entry has defined the property $createRoute.
So I will initialize the method getContentClasses() for the modules where it was missed.

[yurabakhtin]

@luke- I have reviewed all modules and added the missed getContentClasses().

[luke-]

@yurabakhtin @funkycram Sorry for being so late in giving my feedback:

I would actually delete||show already created content at "Always deactivated". The filtering requires a lot of code for quite an edge case. This costs performance and has to be done in other places like search as well. What do you think? Should we stay with the current solution?

@Semir1212 Is "Not available" ok from wording perspective?

[yurabakhtin]

I would actually delete||show already created content at "Always deactivated". The filtering requires a lot of code for quite an edge case. This costs performance and has to be done in other places like search as well. What do you think? Should we stay with the current solution?

@luke- Yes sure, the filtering requires more code so the performance will be slower as before.
So if we will decide to remove all content on set the option to "Not available" we should inform user about the data will be deleted for Spaces or Users completely.

But if we need to keep all content for case when the option was set by mistake or as experiment then we might implement new Content State like Content::STATE_DISABLED = 200. I think for such case we don't need the additional query filters at least in stream, because currently by default all content records are already filtered by content.state => Content::STATE_PUBLISHED.

[luke-]

I would actually delete||show already created content at "Always deactivated". The filtering requires a lot of code for quite an edge case. This costs performance and has to be done in other places like search as well. What do you think? Should we stay with the current solution?

@luke- Yes sure, the filtering requires more code so the performance will be slower as before. So if we will decide to remove all content on set the option to "Not available" we should inform user about the data will be deleted for Spaces or Users completely.

Yes. Maybe something similar to the Private/Public Space Visibility Toggle.

But if we need to keep all content for case when the option was set by mistake or as experiment then we might implement new Content State like Content::STATE_DISABLED = 200. I think for such case we don't need the additional query filters at least in stream, because currently by default all content records are already filtered by content.state => Content::STATE_PUBLISHED.

Sounds good but I'm not sure how to handle changes back to available.

---

Delete is probably the cleanest solution. @funkycram What do you think?

[yurabakhtin]

Sounds good but I'm not sure how to handle changes back to available.

@luke- Ah yes, you are right, it will be not right to restore all records from Content::STATE_DISABLED to Content::STATE_PUBLISHED because some of them were deleted or draft before the module disabling.

[marc-farre]

Delete is probably the cleanest solution. @funkycram What do you think?

Yes, that's okay for me. Do you confirm it's a better solution than #6486 (comment) -> "unless I add a column..."?

something similar to the Private/Public Space Visibility Toggle.

Yes

[luke-]

@funkycram

Yes, that's okay for me. Do you confirm it's a better solution than #6486 (comment) -> "unless I add a column..."?

There is a draft PR which changes the DB structure so the "module_id" would be available. #6217 - Maybe we can change this later,...

[Semir1212]

Yes @luke-

[marc-farre]

@luke- @yurabakhtin commit cd10769

@luke- To confirm module disabling for users and spaces, I didn't choose your modal box solution because the form is already in a modal box. What do you think of the solution of this commit (I tried to make something simple in the code)?

[luke-]

@marc-farre Thanks looks good for me!

@Semir1212 Can you please take a look over the red warning text?
@yurabakhtin Can you please review?

[Semir1212]

@luke- @marc-farre @yurabakhtin

The module is currently being used by X users or spaces. If you change its availability, all content created with the module will be lost. Proceed?

[marc-farre]

Thanks @Semir1212 . Done in commit 5b0d274

@luke- @yurabakhtin full PR: #6499

[luke-]

@marc-farre Thanks, looks fine for me. Is target v1.16 ok for you?

@yurabakhtin Can you please also do a quick review?

[marc-farre]

Yes 1.16 is fine for me. It's only a personal contribution, not a customer request. Thanks for asking!

[yurabakhtin]

@yurabakhtin Can you please also do a quick review?

@luke- I agree the new changes.

[luke-]

Thanks!