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

Feature/api 404 redirect #1837

Merged
merged 16 commits into from
Oct 31, 2016
Merged

Feature/api 404 redirect #1837

merged 16 commits into from
Oct 31, 2016

Conversation

brylie
Copy link
Contributor

@brylie brylie commented Oct 28, 2016

Closes #1261

Changes

  • Show 404 error when API not found
    • Add server method to check for API, so that user doesn't get data before authorization method runs
  • Lint other files/methods

@brylie brylie added this to the Sprint 34 milestone Oct 28, 2016
@marla-singer marla-singer self-assigned this Oct 28, 2016
@marla-singer
Copy link
Contributor

@brylie Self assigned

@marla-singer
Copy link
Contributor

@brylie
Something is wrong with navbar and Latest APIs section. Also my catalog is empty too but I have the apis.
joxi_screenshot_1477655641159

@marla-singer
Copy link
Contributor

marla-singer commented Oct 28, 2016

@brylie Sorry, looks like I have problem on my side

}
}

return apiExists;
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 it can be easy:

if (api) {
  return true
} else {
  return false
}

And placeholder doesn't need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I structured the code as such to make the linter happy. It was complaining that I had an unnecessary 'else' after a return:

http://eslint.org/docs/rules/no-else-return

@@ -19,4 +19,40 @@ Meteor.methods({
Apis.update(apiId, { $push: { authorizedUserIds: user._id } });
}
},
currentUserCanViewApi (apiId) {
// Placeholder for 'user is authorized'
let userAuthorized;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also simplify this one:

 // Get API
const api = Apis.findOne(apiId);
// Check if user can view
return api && api.currentUserCanView()

},
currentUserCanEditApi (apiId) {
// Placeholder for 'user can edit'
let userCanEdit;
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one

 // Get API
const api = Apis.findOne(apiId);
// Check if user can view
return api && api.currentUserCanEdit()

@brylie
Copy link
Contributor Author

brylie commented Oct 28, 2016

Ping @marla-singer. Changes made.

// Check if API exists
Meteor.call('checkIfApiExists', apiId, function (error, apiExists) {
// Check if API exists
if (apiExists === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Edit this condition to

apiExists === undefined 

@brylie
Copy link
Contributor Author

brylie commented Oct 29, 2016

@marla-singer I inverted the conditional, to make it easier to read and avoid the false vs undefined choice.

@marla-singer
Copy link
Contributor

@brylie Nice solution 👍 Works as expected. Merge it

@marla-singer marla-singer merged commit 34e3ab4 into develop Oct 31, 2016
@marla-singer marla-singer deleted the feature/api-404-redirect branch October 31, 2016 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants