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.

Sign up for GitHub

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

Fix rewards extension state (notifications) array attempt to set by named property #2407

Merged
merged 1 commit into from
May 11, 2019
Merged

Fix rewards extension state (notifications) array attempt to set by named property #2407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions ...ve_rewards/resources/extension/brave_rewards/background/reducers/rewards_panel_reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,13 @@ export const rewardsPanelReducer = (state: RewardsExtension.State | undefined, a
return
}

const id = payload.id
let notifications: Record<number, RewardsExtension.Notification> = state.notifications
const id: string = payload.id
let notifications: Record<string, RewardsExtension.Notification> = state.notifications

if (!notifications) {
notifications = []
// Array check for previous version of state types
// (https://github.com/brave/brave-browser/issues/4344)
if (!notifications || Array.isArray(notifications)) {
notifications = {}
}

notifications[id] = {
Expand Down Expand Up @@ -368,7 +370,7 @@ export const rewardsPanelReducer = (state: RewardsExtension.State | undefined, a
break
}
case types.ON_ALL_NOTIFICATIONS: {
const list = payload.list
const list: RewardsExtension.Notification[] = payload.list

if (!list) {
break
Expand All @@ -377,8 +379,10 @@ export const rewardsPanelReducer = (state: RewardsExtension.State | undefined, a
let notifications: Record<number, RewardsExtension.Notification> = state.notifications
let id = ''

if (!notifications) {
notifications = []
// Array check for previous version of state types
// (https://github.com/brave/brave-browser/issues/4344)
if (!notifications || Array.isArray(notifications)) {
notifications = {}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main fix. @NejcZdovc can you explain better about:

  • why this was an array
  • what the repeatable case could be to get in this state where either notification is blank or existing state.notifications is an array (which was my case - state.notifications was already []) so that we can get a test plan

Copy link
Member

Choose a reason for hiding this comment

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

@petemill line 155 in this same file seems to set it, line 168 merges it back into state.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that I copied it from line 156 where we set it to array and I guess that was problematic as well. So I think we need to fix line 156 as well. You can see that in tests that I added it was expected to be an object 9cecb15#diff-f250acb8ae40c4d514754045d9b6e936R379.

Not sure as why only few people see this problem as it should break for everyone

Copy link
Member Author

Choose a reason for hiding this comment

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

@bbondy @NejcZdovc that in L155 also needs to change, right? It's invalid to create this property as an array when it's used as an object.

Copy link
Member Author

@petemill petemill May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@NejcZdovc would only be a problem if state.notifications does not yet exist, right? Do we persist this f/e redux state to disk? Me, personally, I 'upgraded' the profile from dev to nightly before experiencing this bug, if that helps

Copy link
Member Author

@petemill petemill May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The type in L153 also appears to be incorrect, and assume that notification ID is always a number:

let notifications: Record<number, RewardsExtension.Notification> = state.notifications

...but even then it assigns an empty array to that type. I doubt that should pass typescript compile... 🤷‍♂

Copy link
Contributor

Choose a reason for hiding this comment

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

yup we need to change line 155. It's strange why it would work for some and not work for others. Also we had this code (line 155) in there for ages

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember the specific details, but wondering if this is fallout from the notifications type change. I seem to recall they were initially integers and then switched to strings. I wonder if that caused a problem and/or wasn't fully refactored...

}

list.forEach((notification: RewardsExtension.Notification) => {
Expand Down
2 changes: 1 addition & 1 deletion components/definitions/rewardsExtensions.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ declare namespace RewardsExtension {
currentNotification?: string
enabledAC: boolean
enabledMain: boolean
notifications: Record<number, Notification>
notifications: Record<string, Notification>
publishers: Record<string, Publisher>
report: Report
grants?: GrantInfo[]
Expand Down