-
Notifications
You must be signed in to change notification settings - Fork 8
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
User data export #77
User data export #77
Conversation
@yurabakhtin Thanks, looks already quite good! |
@yurabakhtin Thanks, looks very good. What do you think of the following API for modules? We should also document this API in the Legal module under config.php ['class' => 'humhub\modules\legal\services\ExportService', 'event' => 'collectUserData', 'callback' => ['humhub\modules\wiki\Events', 'onLegalUserDataCollect']], Events.php use humhub\modules\legal\events\UserDataCollectionEvent;
public static function onLegalUserDataCollect(UserDataCollectionEvent $event)
{
$event->addData('wiki', array_map(function ($page) {
return RestDefinitions::getWikiPage($page);
}, WikiPage::find()
->joinWith('content')
->andWhere(['content.created_by' => $event->user->id])
->all());
);
# $event->addFile() //?
} |
I have implemened your solution in the commits 4bd8b8d and humhub/wiki@7319a0d. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurabakhtin Thank you, I really like the solution.
What slightly bugs me is that the ExportService
has become a very complex singleton class.
What do you think about introducing a class like events/UserDataCollectionEvent
(maybe based on UserEvent
), which is only responsible for collecting and holding the data.
Then we could remove this part from the ExportService
.
Developers who want to implement the Export Data Feature then only have to deal with the very simple UserDataCollectionEvent
.
@luke- Done in the commits e5c0316 and humhub/wiki@dfa00e0. |
events/UserDataCollectionEvent.php
Outdated
}, Like::findAll(['created_by' => $this->user->id]))); | ||
} | ||
|
||
public function addUserData(string $name, array $data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurabakhtin Can we just name it addData
? Since it's a UserEvent anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luke- I thought about this too and I tried to use there a property name data
instead of current userData
, but the class yii\base\Event
already has the property public $data;
and it is reset to null
after switch between triggers, i.e. if on init method we have the array filled with 5 default items then on go to next module I see the $data
is empty again, so it is why I have decided to use new property with name $userData
and also I have decided to use the the method name as addUserData()
instead of addData()
because I am afraid the method addData()
may be used in future by the class yii\base\Event
and we will may have a conflict in their declarations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurabakhtin Ah ok. Can we rename it to
addExportData($category, $data)
addExportFile($fileName, $sourceFilePath)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
events/UserDataCollectionEvent.php
Outdated
*/ | ||
public function init() | ||
{ | ||
$this->addExportData('user', UserDefinitions::getUser($this->user)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurabakhtin Can we add the data of core here via the Rest module? In the same way as the other modules?
To cover the core data completely in the package, we need to create some more definitions. I think this is easier to do in the REST module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luke- Yes, good idea, done humhub/rest#177 and de4833d.
https://github.com/humhub/humhub-internal/issues/167