-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added function to save grades to local storage and load grades from storage #153
Conversation
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
…orage API Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
…oJson method for class Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Made the changes you mentioned. |
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.
I think this is going very much in the right direction. I like it!
Just a couple comments on implementation.
src/js/helpers.js
Outdated
/** | ||
* Return saved grades for specified username. | ||
* @param {String} username users full name | ||
* @returns {Course[]} list of courses objects for that user | ||
*/ | ||
function getSavedGrades (username) { | ||
const courses = []; | ||
let course_list = {}; | ||
browser.storage.local.get(["user_list"], function (data) { | ||
course_list = data.user_list[username].courses; | ||
}); | ||
for (let i = 0; i < course_list.length; i++) { | ||
courses.push(new Course(course_list[i].name, course_list[i].link, course_list[i].grade, course_list[i].finalPercent, course_list[i].assignments)); | ||
} | ||
return courses; | ||
} |
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.
There's a race condition here if the api does not return fast enough.
Also, the WebExtension API is promised based rather then callback based. I'd recommend turning this whole function into an async function then using await for the storage call.
Also, I just learned about the @async
tag in JSDoc. Nice!
/** | |
* Return saved grades for specified username. | |
* @param {String} username users full name | |
* @returns {Course[]} list of courses objects for that user | |
*/ | |
function getSavedGrades (username) { | |
const courses = []; | |
let course_list = {}; | |
browser.storage.local.get(["user_list"], function (data) { | |
course_list = data.user_list[username].courses; | |
}); | |
for (let i = 0; i < course_list.length; i++) { | |
courses.push(new Course(course_list[i].name, course_list[i].link, course_list[i].grade, course_list[i].finalPercent, course_list[i].assignments)); | |
} | |
return courses; | |
} | |
/** | |
* Return saved grades for specified username. | |
* @async | |
* @param {String} username users full name | |
* @returns {Promise<Course[]>} list of courses objects for that user | |
*/ | |
async function getSavedGrades (username) { | |
const courses = []; | |
const course_list = (await browser.storage.local.get(["user_list"]))?.user_list?.[username]?.courses || []; | |
course_list.forEach(course => { | |
courses.push(new Course(course.name, course.link, course_list.grade, course_list.finalPercent, course_list.assignments)); | |
}) | |
return courses; | |
} |
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.
Does the saveGradesLocally function also need to be made async since it's not returning anything?
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.
Good point, couldn't hurt.
course_list.push(courses[i].toObject()); | ||
} | ||
user_data.user_list[username] = { "courses": course_list }; | ||
browser.storage.local.set(user_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.
You're not merging the user_list
that is already in browser storage so I think you are overwriting for every user every time. This won't work if the system logs on for multiple users.
There's two options here. We could first get from browser storage, merge the data, then set back into browser storage, or we could do something like user_data['USERDATA_' + username]
, in which case, we are taking advantage of how the storage api works.
Or, if you have a better option, please say it!
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.
Don't we need some way of listing all the users that exist? That way for last seen grades we can just query that and list all the users/grades on the system.
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.
We could keep a separate array in browser storage of usernames that have been saved? Just an idea.
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Does this look okay to merge? |
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.
Just noticed a couple bugs but otherwise, we should be good.
src/js/helpers.js
Outdated
*/ | ||
async function saveGradesLocally (username, courses) { | ||
const user_data = {}; | ||
user_data.user_list = {}; |
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.
I don't think this user_list
is necessary anymore.
src/js/helpers.js
Outdated
course_list.push(courses[i].toObject()); | ||
} | ||
user_data["USERDATA_" + username] = { "courses": course_list }; | ||
await browser.storage.local.set(user_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.
await browser.storage.local.set(user_data); | |
browser.storage.local.set(user_data); |
No need to await
here.
src/js/helpers.js
Outdated
user_list.push(username); | ||
const users = {}; | ||
users.user_list = user_list; | ||
await browser.storage.local.set(users); |
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.
await browser.storage.local.set(users); | |
browser.storage.local.set(users); |
same as above
src/js/helpers.js
Outdated
*/ | ||
async function getSavedGrades (username) { | ||
const courses = []; | ||
const course_list = (await browser.storage.local.get("USERDATA_" + username)); |
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.
const course_list = (await browser.storage.local.get("USERDATA_" + username)); | |
const course_list = (await browser.storage.local.get("USERDATA_" + username))["USERDATA_" + username] || []; |
src/js/helpers.js
Outdated
} | ||
user_data["USERDATA_" + username] = { "courses": course_list }; | ||
await browser.storage.local.set(user_data); | ||
const user_list = (await browser.storage.local.get("user_list"), []); |
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.
const user_list = (await browser.storage.local.get("user_list"), []); | |
const user_list = (await browser.storage.local.get({user_list: []})).user_list; |
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Haven't done the UI yet but this adds a function to load grades from local storage and save grades back in.
I also added a method toJson to the Course class so that you can save a course object to chrome local storage.