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(#4848) - Return only isVisible Tags, unless admin #4879

Merged

Conversation

machikoyasuda
Copy link
Contributor

@machikoyasuda machikoyasuda commented Dec 14, 2018

Resolves #4848
Impact: minor
Type: feature

Issue

The current Tags query returns all in the collection, even if a tag's isVisible is set to false. To make sure the public cannot view the tag listing pages of certain tags, the Tags query should default to only returning isVisible tags.

Solution

  • Adds a shouldIncludeInvisible parameter to the main query, set to false by default
  • Add a check for Admin status before shouldIncludeInvisible can return all tags
  • Unit tests

Breaking changes

None

Testing

  1. Mark some tags as isVisible == false
  2. Test that an Admin can get all Tags, including Invisible tags
  3. Test that a non-Admin can only get Visible tags
  4. Test tag and tags queries

@machikoyasuda machikoyasuda changed the base branch from master to release-2.0.0-rc.8 December 14, 2018 01:02
@aldeed aldeed changed the base branch from release-2.0.0-rc.8 to develop December 14, 2018 20:10
@machikoyasuda machikoyasuda changed the title feat(#4848) - WIP - Return only isVisible Tags, unless admin feat(#4848) - Return only isVisible Tags, unless admin Dec 17, 2018
@machikoyasuda machikoyasuda added this to the 🏔 Shavano milestone Dec 17, 2018
@@ -166,6 +167,7 @@ export default function () {
slug: "tag",
position: _.random(0, 100000),
// relatedTagIds: [],
isActive: true,
Copy link
Member

Choose a reason for hiding this comment

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

Should isActive be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooops.... 😅

query.isVisible = { $ne: false };
} else if (shouldIncludeInvisible === true && context.userHasPermission(["owner", "admin"], shopId)) {
query.isVisible = { $in: [false, true] };
}
Copy link
Member

@mikemurray mikemurray Dec 18, 2018

Choose a reason for hiding this comment

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

If shouldIncludeInvisible === true but you're not an admin user, you will end up getting the invisible and visible tags.

What's even more interesting, if shouldIncludeDeleted = true you can see deleted tags as well.

The logic in the function needs a bit of a rework. I think you should check if the user is an admin or not, then decide if they get to see isVisible:true or isDeleted:true tags. Then for everyone else, they don't get to see invisible or deleted products.

if isAdmin
    shouldIncludeInvisible (add proper flags to query)
    shouldIncludeDeleted (add proper flags to query)
else
   only show visible and not delete tags

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

I can see a single tag marked as isVisible=false for the following query as both an admin and guest when shouldIncludeInvisible=false

"hats" was marked as isVisible = false

    tag(slugOrId: "hats") {
      name
      slug
      isVisible
      displayTitle
    }

I think you need to add this same login to the tag (singular) query as well.

@mikemurray
Copy link
Member

👍

@mikemurray mikemurray merged commit 0ee327d into develop Jan 2, 2019
@mikemurray mikemurray deleted the 4848-feat-machikoyasuda-only-return-isVisible-tags branch January 2, 2019 21:03
@spencern spencern mentioned this pull request Jan 18, 2019
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.

2 participants