-
-
Notifications
You must be signed in to change notification settings - Fork 760
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
Implemented check for changes in Wins-form (Responses) spreadsheet within Wins Data automation #7409
base: gh-pages
Are you sure you want to change the base?
Implemented check for changes in Wins-form (Responses) spreadsheet within Wins Data automation #7409
Conversation
…th `iancooperman`
…ata in respository
…te that accidentally committed changes to appsscript.json This reverts commit 1b474c1.
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Proposed changes to Wins Form Admin Guide: https://docs.google.com/document/d/1UTv4msXoThx7AwX7g2c8WgBLnwIJeRkaHePvbvI_3sg/edit?usp=sharing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @iancooperman for your work on this complex issue. On the first line of the PR, the issue number (after "Fixes") is incorrect, Please correct that so the PR will be linked correctly to the issue. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iancooperman Thank you for taking on this issue. The branch name looks good only thing is that I am confused which issue you fixed please correct that if possible. Other than that everything seems to be concise.
@codyyjxn There was a small typo in the linked issue number. Thanks for catching that. Let me know if anything else needs to be done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @iancooperman I'm still testing but I need to ask you to update line 62
@@ -51,7 +59,7 @@ function updateWinsFile(fileName, content, sha) { | |||
function createPR() { | |||
const payload = { | |||
"title": "Update wins-data.json via Google Apps Script", | |||
"head": "elizabethhonest:update-wins-data", | |||
"head": "iancooperman:update-wins-data", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this back to elizabethhonest
const keyValueFile = "_wins-data.json" | ||
const keyValueSha = ghrequests.getSHA(keyValueFile); | ||
const [keyValueSha, keyValueContent] = ghrequests.getWins(keyValueFile); | ||
if (keyValueSha === false) { | ||
console.log('Ending script...') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the console log output message more descriptive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @iancooperman so far the code looks great but
commit 003c436 includes changes to google-apps-scripts/gh-requests/Code.js, which we do not want included in the git repository.
console.log("Entry difference detected. Updating wins file..."); | ||
const writeResponse = ghrequests.updateWinsFile(keyValueFile, encodedKeyValueData, keyValueSha); | ||
if (writeResponse === false) { | ||
console.log('Ending script...') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the console log output message more descriptive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roslynwythe Just to confirm, you're saying to remove the changes to appscript.json
, right? Your initial comment says Code.js
but the commit you linked is to appscript.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roslynwythe Just to confirm, you're saying to remove the changes to
appscript.json
, right? Your initial comment saysCode.js
but the commit you linked is toappscript.json
.
I have assumed this is the case and reverted commit 003c436.
This reverts commit 003c436.
Fixes #4154
What changes did you make?
_wins-data.json
to that of Wins-form (Responses) to see if any changes have been made (e.g. new win, hiding of win, etc.)Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)