Skip to content

Refactor commands that expect HTTP 200 calls with undefined results #3420

Description

@martinlingstuyl

Some time ago I ran into a few planner commands that check if the API responses are undefined:

return request
      .get(requestOptions)
      .then((response: any) => {
        const etag: string | undefined = response ? response['@odata.etag'] : undefined;

        if (!etag) {
          return Promise.reject(`Error fetching task details`);
        }

        return Promise.resolve(etag);
      });

This is accompanied by a test in the testfile:

it('fails validation when task details endpoint fails', (done) => {
    sinonUtil.restore(request.get);

    sinon.stub(request, 'get').callsFake((opts) => {
      if (opts.url === `https://graph.microsoft.com/v1.0/planner/tasks/Z-RLQGfppU6H3663DBzfs5gAMD3o/details`) {
        return Promise.resolve(undefined);
      }

      return Promise.reject('Invalid request');
    });
 
   // etc
});

I wonder if there is some specific reason why this coded this way. If an endpoint fails, it probably does not return HTTP 200, and henceforth this checks and testcases are superfluous and can be removed.

The places where I noticed this are:

  • task add
  • task reference add
  • task reference remove
  • task set

and also in serviceannouncement health get.

I'd think we should refactor this, or it should be clearly stated in a comment that this behavior was observed.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions