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

API blueprint for Custom System Role #5433

Conversation

prabhupant
Copy link
Contributor

Fixes #5297

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

add API blueprints for custom system role

Changes proposed in this pull request:

  • added api blueprints for custom system role

Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

You don't need to write 'Fixed #5297' in the name of the PR, rather write it at the bottom as in the template.

abhinavk96
abhinavk96 previously approved these changes Oct 21, 2018
@abhinavk96 abhinavk96 changed the title Fixes #5297 API blueprint for Custom System Role API blueprint for Custom System Role Oct 21, 2018
Copy link
Member

@bhaveshAn bhaveshAn left a comment

Choose a reason for hiding this comment

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

Travis isn't passing. Please check implementation of other API blueprints thoroughly.

@prabhupant
Copy link
Contributor Author

@bhaveshAn the travis build was failing coz custom system role api needs panel permission in one case and I have added panel permission in a different PR. So, should i delete these 2 PRs and file a new one, combining both the panel permission and custom system role api blueprints?

@bhaveshAn
Copy link
Member

the travis build was failing coz custom system role api needs panel permission in one case and I have added panel permission in a different PR. So, should i delete these 2 PRs and file a new one, combining both the panel permission and custom system role api blueprints?

@prabhupant Nope,

  1. You added blueprint for GET /v1/custom-system-roles twice
    firstly as following which is true due to which it passes the test.
{
            "jsonapi": {
                "version": "1.0"
            },
            "meta": {
                "count": 2
            },
            "links": {
                "self": "/v1/custom-system-roles"
            },
            "data": [
                {
                    "relationships": {
                        "panel-permissions": {
                            "links": {
                                "self": "/v1/custom-system-roles/1/relationships/panel-permissions",
                                "related": "/v1/custom-system-roles/1/panel-permissions"
                            }
                        }
                    },
                    "type": "custom-system-role",
                    "links": {
                        "self": "/v1/custom-system-roles/1"
                    },
                    "attributes": {
                        "name": "Sales Admin"
                    },
                    "id": "1"
                },
                {
                    "relationships": {
                        "panel-permissions": {
                            "links": {
                                "self": "/v1/custom-system-roles/2/relationships/panel-permissions",
                                "related": "/v1/custom-system-roles/2/panel-permissions"
                            }
                        }
                    },
                    "type": "custom-system-role",
                    "links": {
                        "self": "/v1/custom-system-roles/2"
                    },
                    "attributes": {
                        "name": "Marketer"
                    },
                    "id": "2"
                }
            ]
        }

secondly as following due to which it dosn't passes

{
            "jsonapi": {
                "version": "1.0"
            },
            "links": {
                "self": "/v1/custom-system-roles/2"
            },
            "data": {
                "relationships": {
                    "panel-permissions": {
                        "links": {
                            "self": "/v1/custom-system-roles/2/relationships/panel-permissions",
                            "related": "/v1/custom-system-roles/2/panel-permissions"
                        }
                    }
                },
                "type": "custom-system-role",
                "links": {
                    "self": "/v1/custom-system-roles/2"
                },
                "attributes": {
                    "name": "Marketer"
                },
                "id": "2"
            }
        }
  1. PATCH works on /v1/custom-system-roles/<id> and not /v1/custom-system-roles thats why showing 405 Method Not Allowed
  2. DELETE works on /v1/custom-system-roles/<id> and not /v1/custom-system-roles thats why showing 405 Method Not Allowed

Please try to figure it out using travis build

@prabhupant prabhupant force-pushed the 5297-custom-system-role-API-blueprint branch 4 times, most recently from a89798a to 0eb4392 Compare October 28, 2018 16:20
@prabhupant prabhupant force-pushed the 5297-custom-system-role-API-blueprint branch from 0eb4392 to 67d48f5 Compare October 28, 2018 16:52
@codecov
Copy link

codecov bot commented Oct 28, 2018

Codecov Report

Merging #5433 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #5433   +/-   ##
============================================
  Coverage        62.76%   62.76%           
============================================
  Files              261      261           
  Lines            12623    12623           
============================================
  Hits              7923     7923           
  Misses            4700     4700

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15c7698...4399cf8. Read the comment docs.

@prabhupant
Copy link
Contributor Author

@bhaveshAn please review again...the travis build has passed 😃

@bhaveshAn
Copy link
Member

Cool !

@bhaveshAn bhaveshAn merged commit da8f972 into fossasia:development Oct 28, 2018
mrsaicharan1 pushed a commit to mrsaicharan1/open-event-server that referenced this pull request Jan 30, 2019
mrsaicharan1 pushed a commit to mrsaicharan1/open-event-server that referenced this pull request Jan 30, 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.

Add API blueprints for Custom System Roles
3 participants