Initial implementation of /me/drives/sharedByMe endpoint #7239
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
|
Kudos, SonarCloud Quality Gate passed! |
|
converting to draft, as we need to finish the spec PR first. |
rhafer
force-pushed
the
graph-sharedbyme
branch
9 times, most recently
from
086b7b4
to
edc885f
Compare
| } | ||
|
|
||
| render.Status(r, http.StatusOK) | ||
| render.JSON(w, r, &ListResponse{Value: res}) |
There was a problem hiding this comment.
Should we sort the response list so we have reproducible return values?
There was a problem hiding this comment.
I don't know. Should we? I don't think we're doing that elsewhere. Somehow I think we shouldn't. It would only result in clients expecting the results to be returned in a specific order.
| if err != nil { | ||
| return driveItems, errorcode.New(errorcode.GeneralException, err.Error()) | ||
| } | ||
| if lsUserSharesResponse.Status.Code != rpc.Code_CODE_OK { |
There was a problem hiding this comment.
| if lsUserSharesResponse.Status.Code != rpc.Code_CODE_OK { | |
| if lsUserSharesResponse.GetStatus().GetCode() != rpc.Code_CODE_OK { |
use getters to avoid panics
| return driveItems, errorcode.New(errorcode.GeneralException, err.Error()) | ||
| } | ||
| if lsUserSharesResponse.Status.Code != rpc.Code_CODE_OK { | ||
| return driveItems, errorcode.New(errorcode.GeneralException, lsUserSharesResponse.Status.Message) |
There was a problem hiding this comment.
convert lsUserSharesResponse.GetStatus().GetCode() to a http status code?
| case rpc.Code_CODE_NOT_FOUND: | ||
| g.logger.Warn().Str("userid", s.Grantee.GetUserId().GetOpaqueId()).Msg("User not found by id") |
There was a problem hiding this comment.
Return an error? Avoid calling grantedTo.SetUser(*user)?
There was a problem hiding this comment.
🤔 returning an error is probably not right. That would result in the GetSharedByMe to error out just because a single grantee does not exist anymore.
IMO we should just log a warning (like a do now). Though it is debatable if we should return a permission with just the userid (and no displayname) or if we should skip grantees in this case.
rhafer
force-pushed
the
graph-sharedbyme
branch
4 times, most recently
from
708c78b
to
51e173d
Compare
This adds a still incomplete implementation of the new /me/drives/sharedByMe endpoint. For now it only sets the driveItem Id property. The 'permissions' relation is not supported yet. This endpoint is added to the /v1beta1 route, to indicate that it is still imcomplete and might require changes.
getDriveItem does a Stat() call on the resource and sets the corresponding DriveItem properties.
Set the exipration date, grantedToV2 and link attributes on the permissions. The displayName for users and groups isn't expanded yet (only the "id" is set) Also, the "roles" attribute cannot be populated yet.
rhafer
force-pushed
the
graph-sharedbyme
branch
from
51e173d
to
a9c0eeb
Compare
|
Kudos, SonarCloud Quality Gate passed! |
This adds a (still incomplete) implementation of the new /me/drives/sharedByMe endpoint.
Still missing / clarifications needed:
/me/drive/sharedByMeonly includes shares created by the current user.IMO the open items should be tackled in follow-up pull-request. To keep this PR from growing to big.