Fix notifications popup issues

[vantonov1]

No description provided.

[pavgra]

@vantonov1 ,

1. Fix indentation - tabs should be used
2. Use /services/http.js instead of $.ajax
3. Why is jobListing still here if we talked about relying on back-end status? What is going to happen if I refresh page - all notifications gone?

[vantonov1]

3 jobListing is the model for the notifications table, and where are subscribers to jobListing updates
On page refresh notifications are reloaded from backend

[pavgra]

@vantonov1 , if back-end is the single source of truth - why do have lines like sharedState.jobListing.queue(job);?

4. Why commented out lines were not removed?

// this.pollForInfo();
// executionId: String(this.model.currentCohortDefinition().id()) + String(this.getSourceId(source.sourceKey)),

[vantonov1]

Not exactly. Backend is polled, it happens not often, so first notification (with PENDING state usually) is added on UI, and then updated from backend

4 Yes, it have to be removed

[pavgra]

1. The 4 lines of code above look exactly the same way as they do in negative-controls.js. Why not to move it to util method?
2. Why queue here is called outside of timeout function (which seems to be the proper way), while above - inside the timeout?

[pavgra]

Dummy diff

[pavgra]

Get a look at the comment above regarding shared util method

[pavgra]

The same as above

[pavgra]

Not exactly. Backend is polled, it happens not often, so first notification (with PENDING state usually) is added on UI, and then updated from backend

@vantonov1 , wouldn't it be better just throw event / call a method to pull the list of notifications when required but not to maintain two lists (versions) of the notifications?

[vantonov1]

it costs additional server call. After success call to start a job, server returns just enough info to update notifications list - we just add new job to the list, there is no 2 versions. I dont think it is worth to ignore returned info, and ask again

[chrisknoll]

I'm in agreement with @vantonov1 : this is how it works (or used to work? Haven't checked if refactor changed this behavior) in cohort definition generation. The reason: if we rely on the polling to update the status of the generation, and the polling is at 10s intervals, if you post the generate request at second 1 of 10, you have to wait 9 whole seconds to see the UI respond to your generation request. What do users do when they don't see an immediate result of their button click? they click it again. How many clicks can you fit in 9 seconds? A lot (you should see me play starcraft). So, instead, the approach is to get the button click, disable the button, do the async post, get the response, read the status of the job or just populate the job list with a fake (but have the jobID from the initial POST to get the identifier of the job) and then subsequent polls will deliver the up-to-date state.

[pavgra]

@chrisknoll , if you read my previous message, you'd see that you will NOT

have to wait 9 whole seconds

because I suggested

call a method to pull the list of notifications when required

But, I am OK with this:

I dont think it is worth to ignore returned info, and ask again

[chrisknoll]

Apologies, @pavgra , I did not recognize that consideration. However, I will say that I've seen tricky issues with if you have a poll on a setInterval, and then due to a user action you do a invoke-and-wait call to the same action that the setInterval does, you sometimes get this out of order refresh where the setInterval comes back with some slightly different data than your user-initiated call returns and it gets all flaky. So, my underlying concern was just have a single source of refresh (the polling) while being able to update the UI with some state that confirms to they user that we got the request and are working on it. Certainly doing @vantonov1 approach means there is no network latency involved.

But, I am OK with this:

Does that mean you are OK to approve the PR or did you want me to pull and review?

[pavgra]

This causes notification icons to be highlighted on each page load

[pavgra]

@vantonov1 ,

1. Generate cohort
2. Wait until the cohort is generated and notifications are refreshed from server

[pavgra]

@vantonov1 , there is following block of code in cohort-definition-manager.js:

// check if we can tell if the job to generate the reports is already running
					if (this.model.currentCohortDefinition()) {
					var listing = sharedState.jobListing;
						var tempJob = listing().find(this.isActiveJob)
					if (tempJob) {
						if (tempJob.status() == 'STARTED' || tempJob.status() == 'STARTING' || tempJob.status() == 'RUNNING') {
								this.currentJob(tempJob);
								this.generateReportsEnabled(false);
							return "generating_reports";
						}
					}
				}

As soon as we kick off 10 jobs after Cohort reporting (Heracles) it will stop working properly (the required job won't be in the shared state anymore -> need to query for the job status explicitly)