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

Jump to bottom

Fix remove bookmark functionality #1450

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Fix remove bookmark functionality #1450

merged 1 commit into from
Mar 24, 2020

Conversation

rickycodes
Copy link
Member

Resolves #1396

@rickycodes rickycodes changed the title Fix remove bookmark functionality, closes #1396 Fix remove bookmark functionality Mar 23, 2020
@rickycodes rickycodes added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 23, 2020
@rickycodes rickycodes assigned rickycodes and unassigned rickycodes Mar 23, 2020
estebanmino
estebanmino reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

is req always an object or it could be undefined?

@rickycodes
Copy link
Member Author

rickycodes commented Mar 23, 2020 •
edited
Loading

@estebanmino looks like it's always defined. if you look at the rest of the file we're already using req.params[0] and req.params[1] all over the place... pretty sure this was just a mistake (regression)

@danjm-temp
Copy link

Yeah, in the context of the createAsyncMiddleware callback, req should always be defined

@rekmarks
Copy link
Member

json-rpc-engine always errors before calling any middlewares if req is null/undefined, or a non-Object.

ibrahimtaveras00
ibrahimtaveras00 approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Looks good on both OS's, QA Passed 👍

@rickycodes rickycodes merged commit e2b163b into develop Mar 24, 2020
@rickycodes rickycodes deleted the issue-1396 branch March 24, 2020 21:41
rickycodes added a commit that referenced this pull request Jan 31, 2022
@rickycodes
Fix remove bookmark functionality, closes #1396 (#1450)
7104bec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebanmino estebanmino estebanmino approved these changes

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@danjm-temp danjm-temp danjm-temp approved these changes

@rekmarks rekmarks Awaiting requested review from rekmarks

Assignees
No one assigned
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to remove a Favorite URL in the Favorites tab on homepage
5 participants
@rickycodes @danjm-temp @rekmarks @estebanmino @ibrahimtaveras00