Frontend Build Out #41
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| package pactasrv | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/RMI/pacta/cmd/server/pactasrv/conv" | ||
| "github.com/RMI/pacta/db" | ||
| "github.com/RMI/pacta/oapierr" | ||
| api "github.com/RMI/pacta/openapi/pacta" | ||
| "github.com/RMI/pacta/pacta" | ||
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| // Creates an initiative invitation | ||
| // (POST /initiative-invitation) | ||
| func (s *Server) CreateInitiativeInvitation(ctx context.Context, request api.CreateInitiativeInvitationRequestObject) (api.CreateInitiativeInvitationResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| ii, err := conv.InitiativeInvitationFromOAPI(request.Body) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| id, err := s.DB.CreateInitiativeInvitation(s.DB.NoTxn(ctx), ii) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to create initiative invitation", zap.Error(err)) | ||
| } | ||
| if id != ii.ID { | ||
| return nil, oapierr.Internal( | ||
| "failed to create initiative invitation: ID mismatch", | ||
| zap.String("requested_id", string(ii.ID)), | ||
| zap.String("actual_id", string(id)), | ||
| ) | ||
| } | ||
| return api.CreateInitiativeInvitation204Response{}, nil | ||
| } | ||
|
|
||
| // Deletes an initiative invitation by id | ||
| // (DELETE /initiative-invitation/{id}) | ||
| func (s *Server) DeleteInitiativeInvitation(ctx context.Context, request api.DeleteInitiativeInvitationRequestObject) (api.DeleteInitiativeInvitationResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| err := s.DB.DeleteInitiativeInvitation(s.DB.NoTxn(ctx), pacta.InitiativeInvitationID(request.Id)) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to delete initiative invitation", zap.Error(err)) | ||
| } | ||
| return api.DeleteInitiativeInvitation204Response{}, nil | ||
| } | ||
|
|
||
| // Returns the initiative invitation from this id, if it exists | ||
| // (GET /initiative-invitation/{id}) | ||
| func (s *Server) GetInitiativeInvitation(ctx context.Context, request api.GetInitiativeInvitationRequestObject) (api.GetInitiativeInvitationResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| ii, err := s.DB.InitiativeInvitation(s.DB.NoTxn(ctx), pacta.InitiativeInvitationID(request.Id)) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to retrieve initiative invitation", zap.Error(err)) | ||
| } | ||
| result, err := conv.InitiativeInvitationToOAPI(ii) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return api.GetInitiativeInvitation200JSONResponse(*result), nil | ||
| } | ||
|
|
||
| // Claims this initiative invitation, if it exists | ||
| // (POST /initiative-invitation/{id}:claim) | ||
| func (s *Server) ClaimInitiativeInvitation(ctx context.Context, request api.ClaimInitiativeInvitationRequestObject) (api.ClaimInitiativeInvitationResponseObject, error) { | ||
| userID, err := getUserID(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var customErr api.ClaimInitiativeInvitationResponseObject | ||
| err = s.DB.Transactional(ctx, func(tx db.Tx) error { | ||
| ii, err := s.DB.InitiativeInvitation(tx, pacta.InitiativeInvitationID(request.Id)) | ||
| if err != nil { | ||
| return fmt.Errorf("looking up initiative invite: %w", err) | ||
| } | ||
| if !ii.UsedAt.IsZero() || ii.UsedBy != nil { | ||
| if ii.UsedBy != nil && ii.UsedBy.ID == userID { | ||
|
There was a problem hiding this comment. My usual grumbling about new paradigms I don't like: the whole "We use objects for everything and only populate the IDs" makes all of this logic one conditional comparision more than it otherwise would need to be. E.g. as compared to if ii.UsedByID == userID { ... }There was a problem hiding this comment. Yeah I think in most cases you'll come to like this pattern. I've found it increasingly lovely for side projects. |
||
| // We don't return an error if the same user tries to claim the same invitation twice, | ||
| // which might happen by accident, but wouldn't impact the state of the initiative memberships. | ||
| // We may want to log this, though. | ||
|
|
||
| return nil | ||
| } else { | ||
| customErr = api.ClaimInitiativeInvitation409Response{} | ||
| return fmt.Errorf("initiative is already used: %+v", ii) | ||
|
There was a problem hiding this comment. We should be using oapierr.BadRequest (or similar) for this. For this case in particular, we likely want a custom error code so we can show a specific error to the end user. Then you'd just need to update the error logic below to: if errors.Is(err, oapierr.Error{}) { // or something like that, could also make oapierr.Is(err) or similar
return err
} else if err != nil {
return oapierr.Internal(...)
}You could even make a lil helper, like There was a problem hiding this comment. Done, but through different means, as an external var. |
||
| } | ||
| } | ||
| err = s.DB.UpdateInitiativeInvitation(tx, ii.ID, | ||
| db.SetInitiativeInvitationUsedAt(time.Now()), | ||
| db.SetInitiativeInvitationUsedBy(userID)) | ||
| if err != nil { | ||
| return fmt.Errorf("updating initiative invite: %w", err) | ||
| } | ||
| err = s.DB.UpdateInitiativeUserRelationship(tx, ii.Initiative.ID, userID, | ||
| db.SetInitiativeUserRelationshipMember(true)) | ||
| if err != nil { | ||
| return fmt.Errorf("creating initiative membership: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| if customErr != nil { | ||
| return customErr, nil | ||
| } | ||
| return nil, oapierr.Internal("failed to claim initiative invitation", zap.Error(err)) | ||
| } | ||
| return api.ClaimInitiativeInvitation204Response{}, nil | ||
| } | ||
|
|
||
| // Returns all initiative invitations associated with the initiative | ||
| // (GET /initiative/{id}/invitations) | ||
|
There was a problem hiding this comment. Why is this There was a problem hiding this comment. This is getting the invitations associated with an initiative. I think this is the standard paradigm: resources that have a nested/ownership structure can be requested this way. The ID in this case is the initiative ID. There was a problem hiding this comment. Following up on this, I'd just roll "listing invitations" into There was a problem hiding this comment. We won't want that for public access. I think keeping them distinct will make authz much easier. |
||
| func (s *Server) ListInitiativeInvitations(ctx context.Context, request api.ListInitiativeInvitationsRequestObject) (api.ListInitiativeInvitationsResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| iis, err := s.DB.InitiativeInvitationsByInitiative(s.DB.NoTxn(ctx), pacta.InitiativeID(request.Id)) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to list initiative invitations", zap.Error(err)) | ||
| } | ||
| result, err := dereference(mapAll(iis, conv.InitiativeInvitationToOAPI)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return api.ListInitiativeInvitations200JSONResponse(result), nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package pactasrv | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/RMI/pacta/cmd/server/pactasrv/conv" | ||
| "github.com/RMI/pacta/db" | ||
| "github.com/RMI/pacta/oapierr" | ||
| api "github.com/RMI/pacta/openapi/pacta" | ||
| "github.com/RMI/pacta/pacta" | ||
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| // Returns all initiative user relationships for the user that the user has access to view | ||
| // (GET /initiative/{id}/user-relationships) | ||
| func (s *Server) ListInitiativeUserRelationshipsByUser(ctx context.Context, request api.ListInitiativeUserRelationshipsByUserRequestObject) (api.ListInitiativeUserRelationshipsByUserResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| iurs, err := s.DB.InitiativeUserRelationshipsByUser(s.DB.NoTxn(ctx), pacta.UserID(request.UserId)) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to retrieve initiative user relationships by user", zap.Error(err)) | ||
| } | ||
| result, err := dereference(mapAll(iurs, conv.InitiativeUserRelationshipToOAPI)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return api.ListInitiativeUserRelationshipsByUser200JSONResponse(result), nil | ||
| } | ||
|
|
||
| // Returns all initiative user relationships for the initiative that the user has access to view | ||
| // (GET /initiative/user-relationships/{id}) | ||
|
There was a problem hiding this comment. This URL scheme is not intuitive to me, e.g. the difference between:
The benefit of RESTful stuff is that usually it's pretty obvious from the URL what the endpoint does, but I'm not getting that from these. How about something like:
There was a problem hiding this comment.
Yep! Public access won't load relationships. I think this actually fits nicely into the "composite key exception" listed here |
||
| func (s *Server) ListInitiativeUserRelationshipsByInitiative(ctx context.Context, request api.ListInitiativeUserRelationshipsByInitiativeRequestObject) (api.ListInitiativeUserRelationshipsByInitiativeResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| iurs, err := s.DB.InitiativeUserRelationshipsByInitiatives(s.DB.NoTxn(ctx), pacta.InitiativeID(request.InitiativeId)) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to retrieve initiative user relationships by user", zap.Error(err)) | ||
| } | ||
| result, err := dereference(mapAll(iurs, conv.InitiativeUserRelationshipToOAPI)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return api.ListInitiativeUserRelationshipsByInitiative200JSONResponse(result), nil | ||
| } | ||
|
|
||
| // Returns the initiative user relationship from this id, if it exists | ||
| // (GET /initiative/{initiativeId}/user-relationship/{userId}) | ||
|
There was a problem hiding this comment. This API surface seems really broad to me, what's the motivation for having three different endpoints that return a user's relationship to an initiative? Unless we expect the list of relationships to be exceptionally large (>1,000), I'd just let the client filter for the stuff they're looking for. More API surface means more stuff to secure, and more DB functions/tests/code to maintain overall. There was a problem hiding this comment. Yes, they will be large. There is a public initiative, which anyone will have the ability to join, and may be segmented by region (i.e. can't be handled as a standalone case). |
||
| func (s *Server) GetInitiativeUserRelationship(ctx context.Context, request api.GetInitiativeUserRelationshipRequestObject) (api.GetInitiativeUserRelationshipResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| iur, err := s.DB.InitiativeUserRelationship(s.DB.NoTxn(ctx), pacta.InitiativeID(request.InitiativeId), pacta.UserID(request.UserId)) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to retrieve initiative user relationship", zap.Error(err)) | ||
| } | ||
| result, err := conv.InitiativeUserRelationshipToOAPI(iur) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return api.GetInitiativeUserRelationship200JSONResponse(*result), nil | ||
| } | ||
|
|
||
| // Updates initiative user relationship properties | ||
| // (PATCH /initiative/{initiativeId}/user-relationship/{userId}) | ||
| func (s *Server) UpdateInitiativeUserRelationship(ctx context.Context, request api.UpdateInitiativeUserRelationshipRequestObject) (api.UpdateInitiativeUserRelationshipResponseObject, error) { | ||
| // TODO(#12) Implement Authorization | ||
| mutations := []db.UpdateInitiativeUserRelationshipFn{} | ||
| if request.Body.Manager != nil { | ||
| mutations = append(mutations, db.SetInitiativeUserRelationshipManager(*request.Body.Manager)) | ||
| } | ||
| if request.Body.Member != nil { | ||
| mutations = append(mutations, db.SetInitiativeUserRelationshipMember(*request.Body.Member)) | ||
| } | ||
| err := s.DB.UpdateInitiativeUserRelationship(s.DB.NoTxn(ctx), pacta.InitiativeID(request.InitiativeId), pacta.UserID(request.UserId), mutations...) | ||
| if err != nil { | ||
| return nil, oapierr.Internal("failed to update initiative user relationship", zap.Error(err)) | ||
| } | ||
| return api.UpdateInitiativeUserRelationship204Response{}, nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
nothing needs to change here, but because auth in RMI-land is so different than AS-land, we really don't need a whole package and middleware for this, it could just be a server helper like
s.getUserors.getUserID. It's no more or less code in the endpoints themselves, just the difference betweensession.UserID(ctx)ands.getUserID(ctx).