[ENG-10146] Changes in Admin by Support Staff should be visible in activity logs#11697
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.
|
|
||
| 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?
There was a problem hiding this comment.
Yeah, permissions_changed is the dict with user.pk, so technically it would be better to use some iterable (e.g., [user.pk]) instead of a single user.pk. I will update it
| ).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.
There was a problem hiding this comment.
see my first comment
| 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.
There was a problem hiding this comment.
see my comments above
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
⭐ thanks for the detailed response and 👍 for unit tests
cslzchen
merged commit ebde26c
into
CenterForOpenScience:feature/pbs-26-9
3 participants
Ticket
Purpose
add missing logs for admin actions
Changes
Side Effects
QE Notes
CE Notes
Documentation