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

Admin graphpage backend #16298

Closed
wants to merge 54 commits into from
Closed

Admin graphpage backend #16298

wants to merge 54 commits into from

Conversation

howonlee
Copy link
Contributor

@howonlee howonlee commented Jun 1, 2021

Backend bit of #15771

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #16298 (71ab082) into master (260a17e) will increase coverage by 24.23%.
The diff coverage is 96.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #16298       +/-   ##
===========================================
+ Coverage   38.00%   62.24%   +24.23%     
===========================================
  Files        1116     1598      +482     
  Lines       30508    65500    +34992     
  Branches     4634     7226     +2592     
===========================================
+ Hits        11596    40773    +29177     
- Misses      18216    21579     +3363     
- Partials      696     3148     +2452     
Flag Coverage Δ
front-end 37.62% <ø> (-0.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/metabase/models/permissions_group.clj 82.53% <ø> (ø)
src/metabase/models/permissions.clj 80.96% <94.44%> (ø)
src/metabase/api/permissions.clj 55.17% <100.00%> (ø)
frontend/src/metabase/lib/permissions.js 16.80% <0.00%> (-52.11%) ⬇️
...tend/src/metabase/admin/permissions/permissions.js 0.00% <0.00%> (-38.99%) ⬇️
frontend/src/metabase/components/TextInput.jsx 64.70% <0.00%> (-19.51%) ⬇️
frontend/src/metabase/components/tree/Tree.jsx 85.71% <0.00%> (-14.29%) ⬇️
frontend/src/metabase-lib/lib/metadata/Database.js 85.00% <0.00%> (-3.24%) ⬇️
frontend/src/metabase/components/tree/TreeNode.jsx 52.63% <0.00%> (-2.93%) ⬇️
...rs/components/Parameters/syncQueryParamsWithURL.js 90.90% <0.00%> (-0.40%) ⬇️
... and 698 more

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 260a17e...71ab082. Read the comment docs.

@rlotun rlotun requested review from camsaul and a team June 4, 2021 22:53
Comment on lines 29 to 31
(if (some? group_id)
(perms/graph group_id)
(perms/graph)))
Copy link
Member

Choose a reason for hiding this comment

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

https://guide.clojure.style/#body-indentation

You have Common Lisp-style if indentation here... indent both the then and else forms just 2 spaces in Clojure

Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

Overall this LGTM, left a few style notes tho.

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.

None yet

3 participants