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

should this be resilient to sheet name changes? #34

Open
github-actions bot opened this issue Feb 24, 2023 · 3 comments
Open

should this be resilient to sheet name changes? #34

github-actions bot opened this issue Feb 24, 2023 · 3 comments
Assignees
Labels
bug Something isn't working todo

Comments

@github-actions
Copy link

https://github.com/groton-school/bb-lists-to-google-sheets/blob/647c55c73bc400111159c0dce88e9ce1db780181/src/Metadata.ts#L35

        return null;
    }
    const sheet = SpreadsheetApp.getActive().getSheetByName(json.sheet);
    // TODO should this be resilient to sheet name changes?
    return sheet.getRange(json.row, json.column, json.numRows, json.numColumns);
}

const get = (key: string, sheet?: GoogleAppsScript.Spreadsheet.Sheet) => {
    sheet = sheet || SpreadsheetApp.getActive().getActiveSheet();
    return g.SpreadsheetApp.DeveloperMetadata.get(sheet, key);
};

export const getList = (
    sheet?: GoogleAppsScript.Spreadsheet.Sheet
): SKY.School.Lists.Metadata => get(LIST, sheet);
export const getRange = (sheet?: GoogleAppsScript.Spreadsheet.Sheet) =>
    rangeFromJSON(get(RANGE, sheet));
export const getLastUpdated = (sheet?: GoogleAppsScript.Spreadsheet.Sheet) =>
    get(LAST_UPDATED, sheet);

const set = (
    key: string,
    sheet: GoogleAppsScript.Spreadsheet.Sheet = null,
    value: any
) => {
    sheet = sheet || SpreadsheetApp.getActive().getActiveSheet();
    return g.SpreadsheetApp.DeveloperMetadata.set(sheet, key, value);
};

export const setList = (
    list: SKY.School.Lists.Metadata,
    sheet?: GoogleAppsScript.Spreadsheet.Sheet
) => set(LIST, sheet, list);
export const setRange = (
    range: GoogleAppsScript.Spreadsheet.Range,
    sheet?: GoogleAppsScript.Spreadsheet.Sheet
) => set(RANGE, sheet, rangeToJSON(range));
export const setLastUpdated = (
    lastUpdated: Date,
    sheet?: GoogleAppsScript.Spreadsheet.Sheet
) => set(LAST_UPDATED, sheet, lastUpdated.toLocaleString());

const remove = (key: string, sheet?: GoogleAppsScript.Spreadsheet.Sheet) => {
    sheet = sheet || SpreadsheetApp.getActive().getActiveSheet();
    return g.SpreadsheetApp.DeveloperMetadata.remove(sheet, key);
};

export const removeList = remove.bind(null, LIST);
export const removeRange = remove.bind(null, RANGE);
@github-actions github-actions bot added the todo label Feb 24, 2023
@battis battis added the bug Something isn't working label Apr 13, 2023
@battis
Copy link
Member

battis commented Apr 13, 2023

Yes. Yes, it should.

The current behavior is that when the sheet's name is changed, trying to run an update -- even with valid Developer Metadata connecting the sheet to an Advanced List -- is that you get a Google App Script error trying to getRange() on a null value (the sheet that can't be found, since it's only known by name).

Need to either switch to storing sheet by ID (can't remember right now why I'm not doing that) or simply assuming that the sheet affected is the sheet on which the Developer Metadata is stored (a pretty reasonable assumption, one would think).

@battis
Copy link
Member

battis commented Apr 13, 2023

(Ironically, this turned up in a situation where the Advanced List name had also changed, but because that is tracked by ID, not name, it didn't break.)

@battis
Copy link
Member

battis commented Oct 2, 2023

Yes, use the presence of developer metadata to determine that the sheet should be synced. And then use the location of the comment (which should be stored in developer metadata as well as the range last updated) to identify where that range is now.

Could, perhaps, have two comments, one at TL and one at BR, but not sure what to assume if one of them goes missing entirely.

Perhaps if eiter comments goes missing, fail and require the user to manually select the range to be updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working todo
Projects
None yet
Development

No branches or pull requests

1 participant