[ENG-10146] Changes in Admin by Support Staff should be visible in activity logs#11697
[ENG-10146] Changes in Admin by Support Staff should be visible in activity logs#11697antkryt wants to merge 2 commits into
Conversation
|
tests passed on my local branch |
mkovalua
left a comment
mkovalua
left a comment
There was a problem hiding this comment.
LGTM, small changings are needed for Front End side to render foreign_user
An OSF Support Team member made a node private
for logs
|
@adlius try please test job rerun , maybe some CI/CD issue |
antkryt
force-pushed
the
feature/ENG-10146
branch
from
f70f672 to
85f02ee
Compare
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Looks good overall 👍 with a few questions to help me understand and some nitpicking suggestions.
| params = dict(node.log_params) | ||
| params['contributors'] = user.pk |
There was a problem hiding this comment.
I am not familiar with this part, curious on why we change from
params={
'project': node.parent_id,
'node': node.pk,
'contributors': user.pk
}to
params = dict(node.log_params)
params['contributors'] = user.pkIs this a functional change where the the params? e.g. node.log_params may have more than just the three keys.
| permissions=permission, | ||
| notification_type=None | ||
| notification_type=None, | ||
| log=False, |
There was a problem hiding this comment.
Can you help me understand why log=False here? From the ticket, looks like we want changes to be logged but "anonymously" but I may have misunderstood.
There was a problem hiding this comment.
Oh, I might have figured it out. We disable the log here and manually call add_log() later.
|
|
||
| if permissions_changed: | ||
| params = resource.log_params | ||
| params['contributors'] = permissions_changed |
There was a problem hiding this comment.
I am noticed that the type of params['contributors'] can be a integer (user.pk) earlier and a dict here. Seems inconsistent to me?
| ).save() | ||
| auth=None, | ||
| foreign_user=NodeLog.SUPPORT_USER_LABEL, | ||
| params=dict(node.log_params), |
There was a problem hiding this comment.
Similar to an earlier question, node.log_params vs params={'project': node.parent_id}. Is this a required functional change? i.e. passing everything instead of just the one needed.
| params={ | ||
| 'project': node.parent_id, | ||
| 'node': node._primary_key, | ||
| }, |
There was a problem hiding this comment.
Related, why don't use log.params as you did in other places? (a question, not a suggestion)
| params={ | ||
| 'project': node.parent_id, | ||
| 'node': node._primary_key, | ||
| 'pathType': 'file', | ||
| 'path': file_path, | ||
| }, |
There was a problem hiding this comment.
Same here. Maybe there is a reason we have to manually / explicitly set the params in some cases like this and a couple of following ones.
| view = setup_log_view(self.view(), self.request, guid=self.node._id, user_id=self.user_2.id) | ||
| view.post(self.request) | ||
| assert self.node.logs.latest().action != NodeLog.CONTRIB_REMOVED | ||
| assert self.node.logs.filter(action=NodeLog.CONTRIB_REMOVED).exists() |
There was a problem hiding this comment.
Not specific on this one test, should we also test that the anonymous label shows up in the log instead of the admin user.
In addition, we only updated two tests here but here are probably 10 places in the code we updated? Does the other ones don't have a test at all (so nothing to fix)? Should we also update/add tests for each cases.
3 participants
Ticket
Purpose
add missing logs for admin actions
Changes
Side Effects
QE Notes
CE Notes
Documentation