Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change to use everyMinutes trigger for sync #152

Merged

Conversation

mastercko
Copy link
Contributor

Instead of re-setting 'install' script every time, just set an "everyMinutes" trigger of the sync method. Fixes #137

Instead of re-setting 'install' script every time, just set an "everyMinutes" trigger of the sync method.
@derekantrican
Copy link
Owner

Thanks for your suggestion. We had this originally but it's restricted to certain inputs (eg 1, 5, 10, 15, etc) and our solution allowed any number you desired (eg run every 7 minutes) and possibly some other improvements (can't remember). But #137 is definitely a problem (Google's problem, not ours) and stability may be more important. @jonas0b1011001 thoughts?

@jonas0b1011001
Copy link
Collaborator

I don't know what the best option is, but waiting for google to fix the issue might be the worst.

We can go back to the 'old' method, add a setting to use the old method for those that have issues (as this seems to not happen to everyone) or add another check to see if install was successful (as already described here).

@mastercko
Copy link
Contributor Author

Ah, I like the idea of having the flag in the settings for those of us having this problem but allowing the newer method for those that don't.

@Ali1
Copy link

Ali1 commented Sep 30, 2020

Yes please let's count on Google's recurrent triggers! Had problems like #137 and trying to figure out the cause is confusing.

Google trigger + a system for notifying fails will make this app much more dependable even if it means triggers are less frequent than desired.

@Ali1
Copy link

Ali1 commented Sep 30, 2020

Perhaps for people wanting to sync every 7 minutes, a LastRun conditional can be incorporated to enforce this while the Google trigger is everyMinute.

Helpers.gs Outdated Show resolved Hide resolved
Setting the default return value to 15 minutes if the "origFrequency" isn't specified.
Copy link

@Merlin2001 Merlin2001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect from my point of view! 👌
(Unfortunately, I have no write access, either 🙈 )

Helpers.gs Outdated Show resolved Hide resolved
Co-authored-by: jonas0b1011001 <43352574+jonas0b1011001@users.noreply.github.com>
Code.gs Outdated
Comment on lines 94 to 99
throw "[ERROR] \"howFrequent\" must be greater than 0.";
}
else{
ScriptApp.newTrigger("install").timeBased().after(howFrequent * 60 * 1000).create();//Schedule next Execution
ScriptApp.newTrigger("startSync").timeBased().everyMinutes(getValidTriggerFrequency(howFrequent)).create(); //Schedule sync routine to explicitly repeat
ScriptApp.newTrigger("startSync").timeBased().after(1000).create();//Start the sync routine
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous change, the if statement is no longer needed at this point. Please remove it leaving only the trigger creations. After that i think we are good to go.

Helpers.gs Outdated
Logger.log("No valid frequency specified. Defaulting to 15 minutes.");
return 15;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the excessive new line, helpers.gs is quite long already :)

Helpers.gs Show resolved Hide resolved
Input validation for the field was moved into new helper function getValidTriggerFrequency()
Copy link
Contributor Author

@mastercko mastercko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with @jonas0b1011001 's requests.

@derekantrican derekantrican merged commit d7abd14 into derekantrican:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install() function not self-retriggering properly
5 participants