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

refactor: change browser.storage.local structure #243

Merged
merged 3 commits into from
Dec 24, 2020
Merged

refactor: change browser.storage.local structure #243

merged 3 commits into from
Dec 24, 2020

Conversation

priime0
Copy link
Contributor

@priime0 priime0 commented Dec 12, 2020

This refactor aims to make the data a bit more organised so I can later add the opt-out by default feature to displaying last seen grades without having a headache.

browser.storage.local's initial structure looked something like this:

{
    "user_data": {
        "most_recent_user": "name",
        "user": {
            "stuff": "goes here",
        }
    }
}

I altered it to look like this:

{
    "data": {
        "opted_in": true,
        "showExtensionInfo": true,
        "most_recent_user": "name",
        "user_data": {
            "user": {
                "stuff": "goes here",
            }
        }
    }
}

Originally, user_data was the main data stored. I changed it to be a single part of the data that is stored. most_recent_user was inside of user_data, but I felt it didn't fit, so I moved it outside into data.

opted_in is the variable name I replaced for save_last_grades. save_last_grades was originally accessed/created by doing:

browser.storage.local.get({"save_last_grades": true});

When searching through the local storage by supplying an object as the function argument, the fields in the object are used as default values. So, despite save_last_grades not actually being a part of the stored data, it was returned as part of the data. Changing this to search regularly (with the name of the object as the key in the storage) fixes this, as well as adding the field in the instantiation of data.

"showExtensionInfo" is some "legacy" (I'm using this tentatively) stuff Gary added that I'm too lazy to figure out.

Edit: removed commit formatting

This refactor aims to make the data a bit more organised so I can later
add the opt-out by default feature to displaying last seen grades
without having a headache.

`browser.storage.local`'s initial structure looked something like this:

```json
{
    "user_data": {
        "most_recent_user": "name",
        "user": {
            "stuff": "goes here",
        }
    }
}
```

I altered it to look like this:

```json
{
    "data": {
        "opted_in": true,
        "showExtensionInfo": true,
        "most_recent_user": "name",
        "user_data": {
            "user": {
                "stuff": "goes here",
            }
        }
    }
}
```

Originally, `user_data` was the main data stored. I changed it to be a
single part of the data that is stored. `most_recent_user` was inside of
`user_data`, but I felt it didn't fit, so I moved it outside into
`data`.

`opted_in` is the variable name I replaced for `save_last_grades`.
`save_last_grades` was originally accessed/created by doing:
```js
browser.storage.local.get({"save_last_grades": true});
```

When searching through the local storage by supplying an object as the
function argument, the fields in the object are used as default values.
So, despite `save_last_grades` not actually being a part of the stored
data, it was returned as part of the data. Changing this to search
regularly (with the name of the object as the key in the storage) fixes
this, as well as adding the field in the instantiation of `data`.

"showExtensionInfo" is some "legacy" (I'm using this tentatively) stuff
Gary added that I'm too lazy to figure out.

Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
@priime0 priime0 added the enhancement Improvement to a feature label Dec 12, 2020
@gary-kim
Copy link
Member

Sorry, I think I'm confused. What was the problem that was created by having default values?

@priime0
Copy link
Contributor Author

priime0 commented Dec 14, 2020

Hey, @gary-kim .

First, explicitly declaring the structure in the saveGradesLocally function is much clearer; readers are more aware of what data is locally stored. Second, browser.storage.local.get({"save_last_grades": true}) is sort of the same idea as a "magic variable". The save_last_grades field doesn't technically exist in the locally stored data, so it's misleading. Creating a field in the locally stored data called save_last_grades, assigning it a value, then accessing it regularly with browser.storage.local.get("save_last_grades") makes more sense. It also makes it marginally easier to convert it to an opt-out-by-default feature, since the assignment of the value is logically located where the rest of the data is.

opted_in might be too obscure a variable name, though.

@gary-kim
Copy link
Member

gary-kim commented Dec 14, 2020

Ah okay, I get what you're going for. I also thought about making a dedicated function for getting config from that is aware of defaults so that all the defaults are in one place. While I'd prefer that option, it's your and @Suhas-13 's choice.

Also, any chance you can keep info about whether each option was set explicitly so if a default is changed in the future, people who have set their options explicitly don't have it changed under them?
@priime0

`opted_in` and `showExtensionInfo`, instead of storing a boolean value,
now store an object consisting of a `value` and `changed` keys. `value`
indicates which value the user wants for the given option, and `changed`
indicates whether the value has been changed from its default value.
Adding the `changed` key allows for users to have their options kept the
same if the default value is changed.

Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
Added a function `getLocalConfig` to retrieve everything in
browser.storage.local. According to MDN, passing a `null` or `undefined`
value retrieves the entire storage contents, so that is what I did.
Wrapped in a function for better readability and understanding.

In `saveGradesLocally`, I used the `getLocalConfig` function to get the
existing config. Then, checks are performed to see if the config values
exist, and to add them if they don't.

Before, `user_data` also erased previously stored user data. By
attempting to retrieve it from the local storage first, this prevents
that from happening.

Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
@priime0 priime0 merged commit 4f7a0c9 into sas-fossdev:master Dec 24, 2020
@Suhas-13 Suhas-13 mentioned this pull request Dec 31, 2020
@Suhas-13 Suhas-13 added this to the Open Beta v0.22.3 milestone Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants