-
Notifications
You must be signed in to change notification settings - Fork 428
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
Gdpr retreive personal data from mod inbox #2282
Conversation
big_tests/tests/gdpr_SUITE.erl
Outdated
@@ -113,7 +113,21 @@ end_per_testcase(CN, Config) -> | |||
escalus:end_per_testcase(CN, Config). | |||
|
|||
inbox_required_modules() -> | |||
[{mod_inbox, []}]. | |||
[ | |||
{mod_muc_light, [{host, binary_to_list(muclight_domain())}, |
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.
Is it used in the tests anywhere ?
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.
works without so I'll remove it
big_tests/tests/gdpr_SUITE.erl
Outdated
#{ "content" => [{contains, Body}], | ||
"jid" => [{contains, BobS}, | ||
{contains, BobU}], | ||
"unread_count" => "1" } | ||
], | ||
retrieve_and_validate_personal_data( | ||
Alice, Config, "inbox", ExpectedHeader, ExpectedItems) |
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.
Can we assert the inbox of Bob
as well?
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.
yes, we can
What do you think for testing scenarios like retrieve inbox for muclight or retrieve inbox for different values of |
src/inbox/mod_inbox.erl
Outdated
%% gdpr callbacks | ||
%%-------------------------------------------------------------------- | ||
|
||
-behaviour(gdpr). |
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.
It breaks the convention where we have behaviour and exports at the top of the file.
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.
I'll move to the top of the file.
src/inbox/mod_inbox.erl
Outdated
order => asc, | ||
hidden_read => false | ||
}, | ||
Record = mod_inbox_backend:get_inbox(LUser, LServer, InboxParams), |
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.
Definitely not "record". Either "records" or even better - "entries".
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.
I'll rename this variable to Entries
@@ -290,11 +307,11 @@ retrieve_and_validate_personal_data(Alice, Config, FilePrefix, ExpectedHeader, E | |||
}) | |||
end. | |||
|
|||
csv_to_maps(ExpectedHeader, [HeaderRow | [Rows]]) -> | |||
csv_to_maps(ExpectedHeader, [ExpectedHeader | Rows]) -> |
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.
Such fixes should be handled separately, or even directly in base branch, as it affects the logic of all other branches.
e3d7e5e
to
768cee4
Compare
This PR allows to retrieve personal data form
mod_inbox
backends.