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

Change of team permission semantics #569

Merged
merged 9 commits into from
Jan 15, 2019
Merged

Change of team permission semantics #569

merged 9 commits into from
Jan 15, 2019

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Jan 14, 2019

No description provided.

@fisx
Copy link
Contributor Author

fisx commented Jan 14, 2019

@tiago-loureiro what did I miss? I am planning to write one integration test for updateConversation that fails for an external member, would you like to see any other tests?

@fisx
Copy link
Contributor Author

fisx commented Jan 14, 2019

CI doesn't give me the log of the failed test run. Integration tests pass locally for me.

@jschaul
Copy link
Member

jschaul commented Jan 14, 2019

CI doesn't give me the log of the failed test run.

here it is:

(galley)

  add new team member internal:                                FAIL (0.22s)
      test/integration/API/Teams.hs:250:
      billing users
      expected: ["\STX\167\235B%N@r\135\&4\240~\SOH\STX\"#"]
       but got: ["\253\\\136\USA@LG\128\176\&7\204\181}0;","\STX\167\235B%N@r\135\&4\240~\SOH\STX\"#"]

@fisx
Copy link
Contributor Author

fisx commented Jan 14, 2019

thanks! this happens locally:

    add new team member internal:                                OK (0.13s)

i guess i need to run the real aws tests...

@fisx
Copy link
Contributor Author

fisx commented Jan 14, 2019

I am somewhat hopeful that I have made the test pass, but I'm not sure it's still consistent with the intended meaning?

@fisx
Copy link
Contributor Author

fisx commented Jan 14, 2019

Possible further test cases:

  • testUpdateTeamConv, but with creating a non-team conversation
  • corresponding tests for updateConversationAccess, updateConversationReceiptMode, updateConversationMessageTimer.

Should I...? (Preferably in a separate PR.)

@fisx
Copy link
Contributor Author

fisx commented Jan 14, 2019

    update conversation as member:                               FAIL (0.17s)
      test/integration/API/Teams.hs:507:
      status
      expected: 200
       but got: 403

@fisx fisx force-pushed the fisx-hack-team-perms branch from 318fbe1 to e62a698 Compare January 15, 2019 10:15
@fisx
Copy link
Contributor Author

fisx commented Jan 15, 2019

I may have forgotten these:

  • Open/close to guests? (where do i find that?)
  • Create/remove link? (is this POST /conversations/{cnv}/code?)

@jschaul
Copy link
Member

jschaul commented Jan 15, 2019

Open/close to guests? (where do i find that?)

See

-- | Access define how users can join conversations
data Access
= PrivateAccess -- ^ Made obsolete by PrivateAccessRole
| InviteAccess -- ^ User A can add User B
| LinkAccess -- ^ User can join knowing conversation id
| CodeAccess -- ^ User can join knowing [changeable/revokable] code
deriving (Eq, Ord, Show)
-- | AccessRoles define who can join conversations. The roles are
-- "supersets", i.e. Activated includes Team and NonActivated includes
-- Activated.
data AccessRole
= PrivateAccessRole -- ^ Nobody can be invited to this conversation
-- (e.g. it's a 1:1 conversation)
| TeamAccessRole -- ^ Team-only conversation
| ActivatedAccessRole -- ^ Conversation for users who have activated
-- email or phone
| NonActivatedAccessRole -- ^ No checks
deriving (Eq, Ord, Show)
- it's part of the conversation metadata.

Create/remove link? (is this POST /conversations/{cnv}/code?)

yes. But being able to add/remove conversation codes are tied to the access mode of the conversation itself though, not to the person doing the request.

@fisx fisx merged commit eeaeb4c into develop Jan 15, 2019
@fisx fisx deleted the fisx-hack-team-perms branch January 15, 2019 13:00
fisx added a commit that referenced this pull request Jan 16, 2019
This reverts commit eeaeb4c.

The problem with #560 was that the permission I picked to
reuse was one that members did not have, but from here on
we expected them to.
fisx added a commit that referenced this pull request Jan 16, 2019
fisx added a commit that referenced this pull request Jan 16, 2019
* Revert "Change of team permission semantics (#569)"

This reverts commit eeaeb4c.

The problem with #560 was that the permission I picked to
reuse was one that members did not have, but from here on
we expected them to.

* Change of team permission semantics (Fixes #569).
@fisx fisx mentioned this pull request Jan 24, 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