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

Set group and visibility of reply to that of parent #2650

Merged
merged 3 commits into from Oct 22, 2015

Conversation

Projects
None yet
3 participants
@seanh
Contributor

seanh commented Oct 21, 2015

Set the group and visibility of replies to that of their parents, when
replies are first created.

Also prevent the visibility setting of a reply from overwriting the
cached-in-local-storage visibility level. This prevents the following
problem:

  • I'm annotating publicly (my cached-in-local-storage visibility
    setting is public)
  • I reply to a private annotation
  • The visibility of my reply is set to that of its parent, private
  • This visibility is then cached in local storage
  • I make a new top-level annotation, and now it's defaulting to private
    instead of public, even though the user never did anything to change
    the visibility from public to private, it was done automatically
    because they replied to a private annotation.

Obviously changing the mode from private to public when replying to a
public annotation was also possible.

This means that if the user does change the visibility of a reply, we
don't cache that either:

  • I'm annotating publicly (my cached-in-local-storage visibility
    setting is public)
  • I reply to a public annotation
  • The visibility of my reply is set to that of its parent, public
  • This visibility is not cached in local storage because it's a reply
    (and the value cached in the local storage is already the same anyway)
  • I then deliberately change the visibility of my reply from public to
    private
  • Again this visibility is not cached in local storage because it's a reply
  • The next time I make a new top-level annotation, it will still be
    defaulting to public, will not have changed to private mode

@seanh seanh added the WIP label Oct 21, 2015

@seanh seanh self-assigned this Oct 21, 2015

seanh added some commits Oct 21, 2015

Set group and visibility of reply to that of parent
Set the group and visibility of replies to that of their parents, when
replies are first created.

Also prevent the visibility setting of a reply from overwriting the
cached-in-local-storage visibility level. This prevents the following
problem:

- I'm annotating publicly (my cached-in-local-storage visibility
  setting is public)
- I reply to a private annotation
- The visibility of my reply is set to that of its parent, private
- This visibility is then cached in local storage
- I make a new top-level annotation, and now it's defaulting to private
  instead of public, even though the user never did anything to change
  the visibility from public to private, it was done automatically
  because they replied to a private annotation.

Obviously changing the mode from private to public when replying to a
public annotation was also possible.

This means that if the user _does_ change the visibility of a reply, we
_don't_ cache that either:

- I'm annotating publicly (my cached-in-local-storage visibility
  setting is public)
- I reply to a public annotation
- The visibility of my reply is set to that of its parent, public
- This visibility is not cached in local storage because it's a reply
  (and the value cached in the local storage is already the same anyway)
- I then deliberately change the visibility of my reply from public to
  private
- Again this visibility is not cached in local storage because it's a reply
- The next time I make a new top-level annotation, it will still be
  defaulting to public, will not have changed to private mode
Rename "public" -> "shared"
Rename permissions.public() -> permissions.shared() and
permissions.isPublic() -> permissions.isShared().

This is more consistent with language used elsewhere in the code, now
that we have groups.

@seanh seanh removed the WIP label Oct 21, 2015

@seanh seanh removed their assignment Oct 21, 2015

@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Oct 21, 2015

Contributor

This is done, who wants to review it? @nickstenning @robertknight

Contributor

seanh commented Oct 21, 2015

This is done, who wants to review it? @nickstenning @robertknight

@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Oct 21, 2015

Contributor

This should also fix the problem with replying in the stream. No matter what group you had focused last time you used the sidebar, replies in the stream should get the same group as their parents

Contributor

seanh commented Oct 21, 2015

This should also fix the problem with replying in the stream. No matter what group you had focused last time you used the sidebar, replies in the stream should get the same group as their parents

@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 22, 2015

Contributor

This looks great. Nice and clean. One minor thing: the publish-annotation-btn directive no longer needs an isNew param, so that can be removed (from the directive, its tests, and annotation.html).

Contributor

nickstenning commented Oct 22, 2015

This looks great. Nice and clean. One minor thing: the publish-annotation-btn directive no longer needs an isNew param, so that can be removed (from the directive, its tests, and annotation.html).

@@ -691,6 +751,44 @@ describe("AnnotationController", ->
it("creates the directive without crashing", ->
createAnnotationDirective({})
)
it("sets the permissions of new annotations from local storage", ->

This comment has been minimized.

@robertknight

robertknight Oct 22, 2015

Contributor

here are only two cases here so it doesn't make much difference, but in general I prefer that when you have several tests with very similar logic, they be expressed as:

var cases = [...]
cases.forEach(function (case) {
it('does something for case ' + case.name, function () {
  // create variant using the case
});
}
@robertknight

robertknight Oct 22, 2015

Contributor

here are only two cases here so it doesn't make much difference, but in general I prefer that when you have several tests with very similar logic, they be expressed as:

var cases = [...]
cases.forEach(function (case) {
it('does something for case ' + case.name, function () {
  // create variant using the case
});
}
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Oct 22, 2015

Contributor

LGTM - Just the trivial comment nick mentioned.

Contributor

robertknight commented Oct 22, 2015

LGTM - Just the trivial comment nick mentioned.

nickstenning added a commit that referenced this pull request Oct 22, 2015

Merge pull request #2650 from hypothesis/trello-143-make-replies-defa…
…ult-to-group-and-visibility-of-parent

Set group and visibility of reply to that of parent

@nickstenning nickstenning merged commit 4f6564d into master Oct 22, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nickstenning nickstenning deleted the trello-143-make-replies-default-to-group-and-visibility-of-parent branch Oct 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment