Fix Activity Stream Responses for Records that Never Existed – DEV-17363

[elenamujal]

Checked total_pages and count are 0 in entity Activity Stream, and constructed a 404 Not Found error response accordingly.

[bluebinary]

Hi Elana, thank you for this update, the changes look great, and I tested the functionality and it works as expected both for records that never existed as well as for records that previously did exist but have since been deleted.

In reviewing the output from the updated functionality, I noticed a minor related issue; It isn't something that we need to address here, but I think it is worth noting and discussing further, especially as this is now an open source project, as the clearer we can be throughout, the more user friendly the system becomes.

I noticed that the error response generated by the construct_error_response() helper method, when called using the status_page_not_found named-tuple, produces a "details" message string that is specific to its original use case, and reads "Page number out of bounds" in the JSON response:

{
  "errors": [
    {
      "status": 404,
      "title": "Page Not Found",
      "detail": "Page number out of bounds"
    }
  ]
}

While the wording of the "details" message is appropriate for some use cases, I think it could be improved upon for this use case in a future update by amending the message to broaden the use cases to which it is applicable or by providing a custom "details" message for this use case to ensure clarity for end users.

[elenamujal]

Hi Daniel! Thanks for taking a look at the PR! I completely agree about the error message being unclear, I went through a couple others in the errors file but none seem to be quite right. The closest in my opinion is

status_record_not_found = status_nt(
   404, "Record Not Found", "Unable to obtain matching record from database"
)

although it is not quite right as we are not checking a record in this case. Would it make sense to create a new error with similar language but for an entity rather than a record? We can also discuss further and make that update later. Thanks again!

[bluebinary]

Hi Elana, thank you for the update, much better to deal with those integers as integers.

I agree based on your notes that determining the best error response message can be left for a future update as none of the available options seem to fit here; I think it would be helpful to have the ability to override the "details" part of the error message in the call to construct_error_response() allowing us to customize the message for specific use cases where needed to provide clearer messaging to end users.

Perhaps something like this could work (note the addition of the detail "kwarg" to the method):

def construct_error_response(status, source: int = None, detail: str = None):

    err = {}
    err["status"] = status.code

    # include 'line_number' only when needed
    if source:
        err["source"] = {"line number": source}

    err["title"] = status.title
    err["detail"] = status.detail
    err["detail"] = detail or status.detail  # added support here for the optional `detail` kwarg
    err = [err]
    result = {"errors": err}

    logger.error(err)

    response = current_app.response_class(
        response=json.dumps(result), mimetype="application/json", status=status.code
    )

    if status.code == 503:
        response.headers["Retry-After"] = "30"

    return response

We could then override the default message associated with a named-tuple, when calling construct_error_response(), similar to the following (where the "details" wording is to be adjusted accordingly):

response = construct_error_response(
    status_page_not_found,
    detail="An Activity Stream is only available for records that currently exist or previously existed",
)
return abort(response)

[bluebinary]

The changes look great Elena, thank you!