Feature/implement update delete service request #47
Conversation
Zheng-Zhi-Qiang
added
backend
There was a problem hiding this comment.
Nice LGTM. I skim through the code so do have josh proof read them. Also not sure if we allow service requests to be deleted via API (i.e. to keep for logs and auditing purpose) so do check with josh on that. If its not allowed might be dangerous to expose that API, esp if want to use in DSTA
backend/src/api/handlers/handlers.go
Outdated
| event.FireAsync(events.NewNewServiceRequestEvent(srm)) | ||
| w.WriteHeader(http.StatusCreated) | ||
| } | ||
|
|
||
| func DeleteServiceRequest(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
we won't allow for deletion. this is for logging purposes. but we can have a cancel. however, the cancellation logic will be much more involved and implemented at a later date.
…ement-update-delete-service-request
…under sr cancellation handler
backend/src/api/handlers/handlers.go
Outdated
| return | ||
| } | ||
| srm := &dbmodels.ServiceRequestModel{ | ||
| CreatedOn: time.Now(), |
There was a problem hiding this comment.
you should not update created_on field
| update := bson.M{"$set": bson.M{ | ||
| "pipeline_id": srm.PipelineId, | ||
| "pipeline_version": srm.PipelineVersion, | ||
| "last_updated": srm.LastUpdated, | ||
| "remarks": srm.Remarks}} |
There was a problem hiding this comment.
this is flawed actually.
pipeline_id and version should never be updated. It's a fixed 1-to-1 mapping.
The stuff that can be updated are the user parameters to the form input. None of these parameters you use are updatable.
| Status: dbmodels.Pending, | ||
| } | ||
| err = json.NewDecoder(r.Body).Decode(srm) | ||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
actually why do we still set this here when you already set it inside the JSONError method?
There was a problem hiding this comment.
I specify in case we need to return a response that is not an error.
There was a problem hiding this comment.
okay. then maybe we need a new method for returning data
| if status != models.Pending && status != models.Running { | ||
| logger.Error("[CancelStartedServiceRequest] Unable to cancel service request as execution has been completed", nil) | ||
| JSONError(w, handlermodels.NewHttpError(errors.New("service request execution has been completed")), http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
why not just check for status == models.Completed?
There was a problem hiding this comment.
because the cancellation can happen in the middle of the execution, where maybe the execution is at a pending approval step, and if the status is already completed, means the sr execution completed already what so what is there to cancel
| @@ -33,6 +33,31 @@ func (sr *ServiceRequest) GetById(id string) (*models.ServiceRequestModel, error | |||
| return srm, nil | |||
| } | |||
|
|
|||
| func (sr *ServiceRequest) DeleteById(id string) (*mongo.DeleteResult, error) { | |||
| func (sr *ServiceRequest) GetStatus(id string) (status models.ServiceRequestStatus, err error) { | ||
| srm, err := sr.GetById(id) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return srm.Status, nil | ||
| } |
There was a problem hiding this comment.
I think this method shouldn't exist for a few reasons:
- It provides an unnecessary wrapper around the
GetByIdmethod. - In the event of error, it returns an empty string which is not helpful
You should just call the GetById method directly
…ending the finalisation of the data form
Zheng-Zhi-Qiang
deleted the
feature/implement-update-delete-service-request
branch
Description
In this PR, I implemented delete and updating of service requests using their ids.
Assumptions
For update, I am assuming that only the three of the service request fields will be modified by user: pipeline_id, pipeline_version, and remarks.
Minor Changes
Added logging to all service request handler methods
Post Review Fixes:
*Note: cancellation function to be implemented separately