Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Make default ACL server-based #329

Merged
merged 6 commits into from
Apr 7, 2017
Merged

Conversation

b123400
Copy link
Contributor

@b123400 b123400 commented Mar 24, 2017

@oursky-travis
Copy link
Collaborator

@b123400, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ben181231, @cheungpat and @limouren to be potential reviewers.

@cheungpat
Copy link
Contributor

@b123400 Make sure that your PR title and comment is public consumable because the PR title and comment is usually included in the merge commit.

Specifically “a server side thing” is not good for public understanding, though I totally understand what you are talking about.

@b123400 b123400 changed the title Make default ACL a server side thing Make default ACL a server-based Mar 29, 2017
@b123400 b123400 changed the title Make default ACL a server-based Make default ACL server-based Mar 29, 2017
for _, v := range payload.RawDefaultAccess {
ace := skydb.RecordACLEntry{}
if err := (*skyconv.MapACLEntry)(&ace).FromMap(v); err != nil {
return skyerr.NewInvalidArgument("invalid _default_access entry", []string{"_default_access"})
Copy link
Contributor

Choose a reason for hiding this comment

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

The argument name should be default_access, right?

CREATE TABLE _record_default_access (
record_type text NOT NULL,
default_access jsonb,
UNIQUE (record_type, default_access)
Copy link
Contributor

Choose a reason for hiding this comment

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

The unique constraint should be on record_type only ?

@ben181231
Copy link
Contributor

@b123400 @rickmak @cheungpat Do we need a default ACL for all record types ?

@b123400
Copy link
Contributor Author

b123400 commented Apr 6, 2017

Updated to fix the issue

@ben181231 ben181231 merged commit 8a7f5be into SkygearIO:master Apr 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants