search

trolster / urcli

CLI tool for the Udacity Reviews API
MIT License
100 stars 23 forks source link

Feature Request: add service request id, status and updated_at fields in UI. #83

Closed optimistanoop closed 7 years ago

optimistanoop commented 7 years ago

I'm opening this issue because:

What's going wrong?

urcli assign 301 created a new service request instead refreshing available SR.

How can the urcli team reproduce the problem?

This issue could be a special case, Here is my scenario:

Step 1- Ran urcli assign 301 and it was running since 5-6 hrs - current pos was 3 for last 2-3 hrs and it was not decreasing.

Step 2- I hit ESC and ran urcli assign 301 again, it started a new service request having current pos 46 (it was supposed to refresh old service request).

Step 3- Checked for assigned project too but there was no project assigned to me.

Problem: In the above process, the tool was supposed to refresh service request, but it didn't. It created a new service request.

I am not sure about the reason behind the issue, Following are the possible cases according to me-

supporting information:

What's the feature?

Feature 1- It would be helpful if we have service request id, status and updated_at fields ( taken from refresh API response) in UI. Field id could be service request id and project name could have both id and name like PROJECT_NAME - PROJECT_ID

Feature 2- We can have urcli assign history which can give us last 3-5 used service request id -OR-
It can give us service request id with last found status (this could be done by updating service request history once it is closed or fulfilled).

Feature 3- We can check status in refresh API response once after refresh API hit to update UI. If the status is closed then it can open a new service request with a message on UI saying "previous service request - - was closed and a new service request is opened - ".

What problem is the feature intended to solve?

Feature 1- This will help us in case we fall in the above-specified scenario or in any new issue where we end up having no previous service request to debug. It would have been helpful if I would have had my last service request so that I can check when it got closed as now my new service request is generated and I can't see my old service request. This can help us be sure that service request is available.

Feature 2- This is an advanced enhancement to feature 1.

Feature 3- This will be helpful in the scenario explained above or in any new issues related to refresh API, poor internet, machine sleep, tool crash etc.

Thanks for the great tool. I love urcli.

trolster commented 7 years ago

Hi Anoop, thanks for opening an issue on this. It does sound like a bit of a special issue, and I agree that without the info you are requesting it's really hard (if not impossible) to debug.

The scenarios you describe don't make much sense to me, in terms of what could have happened. urcli can't just delete you sr without you hitting the CTRL-C keys :-). And a closed sr is not kept around on the server, so it can't be update. The server simply responds that there is no active sr, and urcli creates a new one. So the bug would have to go a bit deeper.

What your feature request reminds me of is that I've always wanted to implement logging. There even is a dependency to a logging module that I totally forgot about, so I must have been working on it at some point! Embarrassing :blush:.

I'll keep this feature request around for now and see if anyone else comments. I'd like to hear if there are other ideas that are relevant to this. And I'll update the --debugging flag for the assign command over the weekend, so that it actually does something useful. It will also need to be documented. I know that a debug flag won't solve anything, since you will have to always have it turned on, but it may be a start for those who see some strange behaviour.

optimistanoop commented 7 years ago

Hi @trolster ,

Thanks for the reply. šŸ‘

This issue feels interesting to me too, this is a great tool and hoping we can solve this mystery. šŸ˜„

I have some queries - 1- Does urcli checks for the status of SR every time before or after refreshing it? For example- In my case when assign is running fine and meanwhile, (correct me if I am wrong)

2- I hope In the case described above urcli would have created a new SR if the old SR was closed, (I am not sure whether this feature is available?) but it was updating the same old SR. (Here the mystery lies) We are not sure why this was happening?

FYI: I tried hitting refresh API for a fulfilled/closed SR on swagger and it returns 

{
  .........
  "closed_at": "2017-06-07T16:05:10.352Z",
  "created_at": "2017-06-07T05:36:16.555Z",
  "updated_at": "2017-06-07T16:05:10.364Z",
  "status": "fulfilled"
}

so, if somehow we refresh a fulfilled/closed SR and we don't check for status, we may end up refreshing that SR infinitely and it will not create a new SR in this case.

Hope this explained my concern.

trolster commented 7 years ago

I understand you concern. But to avoid exactly that scenario, urcli doesn't use the id of the SR to check it's status. It simply gets the active SR using the me/submission_requests endpoint. The description of that endpoint is, "get the active submission request for the currently authenticated reviewer." In other words, there is no chance that urcli is looking at a fulfilled request and trying to refresh it, since it's only ever looking at active requests.

The endpoint returns, "The active (status == 'available') submission request objects. Under current constraints this should always be zero or one such requests."

Here is the relevant code:

async function requestLoop() {
  if (env.update) {
    try {
      // We have to check for new assignments every time, to account for edge
      // cases where, for instance, a review is completed and a new one is
      // assigned, all between updates.
      await checkAssigned();
      // We only need to deal with the submission request and the queue if we
      // haven't got max number of submissions assigned.
      if (env.assigned.length < 2) {

        // Here is where the SR gets called.
        const res = await api({task: 'get'});
        const submissionRequest = res.body[0];
        // If a submission_request exists, we save it and check if it should be
        // refreshed.
        if (submissionRequest) {
          env.submission_request = submissionRequest;
          if (env.refresh && env.tick !== 0) {
            await refreshSubmissionRequest();
          }
        } else {
          await createNewSubmissionRequest();
        }
        if (env.updatePositions) {
          await checkPositions();
        }
      }
      // Check if the info needs to update
      if (env.updateFeedbacks && env.flags.feedbacks) {
        checkFeedbacks();
      }
      if (env.flags.ui) {
        setPrompt();
      }
    } catch (e) {
      env.error = e;
    }
  }
  // Set/reset to the default update intervals.
  env.update = env.tick % env.updateInterval === 0;
  env.refresh = env.tick % env.refreshInterval === 0;
  env.updatePositions = env.tick % env.updatePositionsInterval === 0;
  env.updateFeedbacks = env.tick % env.updateFeedbacksInterval === 0;
  env.tick += 1;
  setTimeout(async () => {
    await requestLoop();
  }, 1000);
}

It seems bulletproof to me, but there may of course be issues on the server side that I can't control.

gabraganca commented 7 years ago

Step 1- Ran urcli assign 301 and it was running since 5-6 hrs - current pos was 3 for last 2-3 hrs and it was not decreasing.

Step 2- I hit ESC and ran urcli assign 301 again, it started a new service request having current pos 46 (it was supposed to refresh old service request).

Maybe something else happened here besides having the request deleted, and a new one created. A lot of reviewers are complaining that they are being pushed back to the end of the queue without being assigned a review. Maybe this is what happened to OP. This issue is a probably a bug on the server side since it has happened with different scripts.

optimistanoop commented 7 years ago

@trolster @gabraganca

Thanks for clarifying my doubts. You guys are awesome. Thanks for building this great tool.

trolster commented 7 years ago

Current version has the --verbose flag!! I can't believe that I forgot about that, but now I've documented it also. Simply run urcli assign <id> --verbose. You can also press the key o (for options) and select the "Show debugging information". You get an output that looks something like this:

screen shot 2017-06-09 at 09 21 16

Let me know if you think this is enough to be able to debug.

optimistanoop commented 7 years ago

Wow !! This is really helpful. Thanks.

trolster commented 7 years ago

Since logging is beyond the scope of this issue, and the --verbose flag actually outputs the desired information, I'm closing this.

optimistanoop commented 7 years ago

Thanks for your support. šŸ‘

optimistanoop commented 7 years ago

@trolster Verbose mode helped this time.

The same issue occurred once again, but this time I have some clear data, thanks for verbose mode.

1- In the verbose mode - urcli was showing last updated time of 3hr back, status was available. 2- checked on swagger - status was closed and SR was last updated at the same time it was showing in step 1.

A new SR was supposed to be created once last SR was closed due any internet issue (thinking to get a new internet connection šŸ˜„ ), But UI was still showing the status and response when it lost connection. It never updated UI nor created new SR.

I guess when the internet came back after some hrs, urcli freezes.

Hope this helps.

trolster commented 7 years ago

Hi @optimistanoop, this is good information. Do you have a screenshot?

It would be helpful if you opened a new issue as a bug report.

Thanks again.

optimistanoop commented 7 years ago

@trolster sorry, I don't have the screenshot.

I will try to add screenshot once this happens again, meanwhile, I will create a new issue.

Thanks for your quick reply. šŸ˜„