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

feat: #1848 filter app lists when using the marketplace in Developer Edition #1851

Conversation

trankhacvy
Copy link
Contributor

Pull request checklist

Does this close any currently open issues?

fixes: #1848

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (yarn build) was run locally and any changes were pushed
  • Lint (yarn lint) has passed locally and any fixes were made for failures
  • Test (yarn test) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

Sorry, something went wrong.

@github-actions github-actions bot added cognito-auth Issue relates to Cognito Auth NPM package marketplace Relates to the Marketplace labels Jun 26, 2020
@auto-assign auto-assign bot requested review from nphivu414 and willmcvay June 26, 2020 08:35
@trankhacvy trankhacvy force-pushed the feat/1848-filter-apps-when-using-the-marketplace-in-developer-edition branch from 3a88ff0 to 122ed8a Compare June 26, 2020 08:50
@github-actions github-actions bot added react-app-scaffolder Relates to React App Scaffolder package smb-onboarder Relates to Small Medium Business onboarding app labels Jun 26, 2020
@@ -19,6 +19,7 @@ export const mockLoginSession: LoginSession = {
userCode: 'SOME_USER_CODE',
isAdmin: false,
userTel: '123',
groups: ['AgencyCloudDeveloperEdition'],
Copy link
Contributor

Choose a reason for hiding this comment

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

should use const COGNITO_GROUP_DEVELOPER_EDITION

@@ -66,6 +66,7 @@ const appState: ReduxState = {
adminId: null,
userCode: 'testUserCode',
userTel: '123',
groups: ['AgencyCloudDeveloperEdition'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

* AgencyCloudDeveloperEdition group, if not just return null
* refer to this ticket https://github.com/reapit/foundations/issues/1848
*/
export const selectDeveloperId = (state: ReduxState) => {
Copy link
Contributor

@phmngocnghia phmngocnghia Jun 26, 2020

Choose a reason for hiding this comment

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

Naming seems ambiguous to me even with comment on the func. Imagine we will need to selectDeveloperId in the future without this logic. Something like selectDeveloperIdIfBelong(group) would be better. Just a suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

@phmngocnghia's comment makes sense, and should add return value type too.
It may cause confusion when we splitting to Client/Dev portal next week

Copy link
Contributor

Choose a reason for hiding this comment

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

selectDeveloperEditionId as a name maybe?

Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

Agree with the comments already made, others look good though!

* AgencyCloudDeveloperEdition group, if not just return null
* refer to this ticket https://github.com/reapit/foundations/issues/1848
*/
export const selectDeveloperId = (state: ReduxState) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

selectDeveloperEditionId as a name maybe?

Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@trankhacvy trankhacvy merged commit 28d03c6 into master Jun 26, 2020
@trankhacvy trankhacvy deleted the feat/1848-filter-apps-when-using-the-marketplace-in-developer-edition branch June 26, 2020 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cognito-auth Issue relates to Cognito Auth NPM package marketplace Relates to the Marketplace react-app-scaffolder Relates to React App Scaffolder package smb-onboarder Relates to Small Medium Business onboarding app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a developer I should only see my apps when using the marketplace in Developer Edition
4 participants