fix(UI): share links not storable#198
fix(UI): share links not storable#198utnapischtim wants to merge 1 commit intoinveniosoftware:masterfrom
Conversation
utnapischtim
force-pushed
the
fix-shared-links-with-anonymous
branch
from
99cbc61 to
ee7bb64
Compare
| elif user_id == "None": # why str? | ||
| return self.anonymous_user({"id": user_id}) |
There was a problem hiding this comment.
Should we expose "None" as the user id in the serialized output?
Would using a clearer sentinel like "anonymous" be better searchable in logs then None?
There was a problem hiding this comment.
yeah, that is a good question. i thought that too, but since the user_id should be an int anonymous would be wrong and yes None is also wrong, but at least more in the way of a None type
There was a problem hiding this comment.
It is "None" because here it gets typecast into string. I guess the resolver here should handle both? None and "None" or maybe instead "" and empty string should be passed from the audit log builder?
There was a problem hiding this comment.
which values can user_id have? or better which values can identity.id have?
There was a problem hiding this comment.
From what I can find in the codebase, it could only be the user ids, 'system' and now we will have 'None' for anonymous user.
| username = fields.Constant(_("Deleted user"), dump_only=True) | ||
|
|
||
|
|
||
| class AnonymousSchema(Schema): |
There was a problem hiding this comment.
Should this schema inherit from BaseGhostSchema like UserGhostSchema to keep the serialized structure consistent across special user types?
There was a problem hiding this comment.
BaseGhostSchema has a is_ghost attribute which in my thinking doesn't apply to an AnonymousUser. For me it should not inherit from BaseGhostSchema
| dump_only=True, | ||
| ) | ||
| username = fields.Constant(_("Anonymous user"), dump_only=True) | ||
| email = fields.Constant("anonymous_user@anonymous.ch", dump_only=True) |
There was a problem hiding this comment.
Following the same approach as SystemUserSchema. I would propose
| email = fields.Constant("anonymous_user@anonymous.ch", dump_only=True) | |
| email = fields.Constant("anonymous@inveniosoftware.org", dump_only=True) |
* if the user_id is of type str and has the value 'None' it fails with ValueError on converting to int with int() * this happens when an anonymous user tries to save a draft over a shared link * this fix is not ideal since it assumes that if the user_id is of type str and has the value 'None' it should be an anonymous user
utnapischtim
force-pushed
the
fix-shared-links-with-anonymous
branch
from
ee7bb64 to
0cb9e4a
Compare
3 participants
the problem is that the anonymous users are not handled for shared
links in the audit logs
this creates the situation that 'None' is given to int() which expects
to convert a stringified number and fails
this fix is not ideal since it assumes that if the user_id is of type
str and has the value 'None' it should be an anonymous user
closed: inveniosoftware/invenio-app-rdm#3338