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

[WIP] Add permissions groups store, handlers and login functionality #168

Closed
wants to merge 9 commits into from

Conversation

speza
Copy link
Contributor

@speza speza commented Feb 3, 2019

A WIP. Will eventually resolve #164.

@codecov-io
Copy link

codecov-io commented Feb 4, 2019

Codecov Report

Merging #168 into master will decrease coverage by 1.08%.
The diff coverage is 26.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   62.82%   61.73%   -1.09%     
==========================================
  Files          48       48              
  Lines        3844     3936      +92     
==========================================
+ Hits         2415     2430      +15     
- Misses       1028     1105      +77     
  Partials      401      401              
Impacted Files Coverage Δ
store/permission.go 33.33% <0.00%> (-49.28%) ⬇️
handlers/permission.go 26.22% <23.72%> (-73.78%) ⬇️
handlers/handler.go 91.02% <100.00%> (+0.61%) ⬆️
handlers/user.go 37.81% <100.00%> (-1.87%) ⬇️
store/store.go 74.64% <100.00%> (+0.36%) ⬆️

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 ec28241...e880803. Read the comment docs.

@Skarlso
Copy link
Member

Skarlso commented Apr 27, 2020

@speza Hey man. :) Do you have anything more to add to this by any chance? :)

Sorry, there is now a lot of merge conflict on this. :)

@speza
Copy link
Contributor Author

speza commented Apr 27, 2020

Hey @Skarlso! Good question! Apologies, life has gotten in the way of doing anything open source at the moment.

I guess this comes down to whether we need this enhanced functionality at this stage?

I'd love to get back into it at some point. So I could attempt to fix them sometime or rework the whole change. Its been a while since I delved into here though.

@Skarlso
Copy link
Member

Skarlso commented Apr 27, 2020

I think it's a good to have thing if we want to take things seriously. :) So if you need any help just let me know. Also, I'm very sorry, but I'm about to merge a large change to the singleton service thing. :D

@speza
Copy link
Contributor Author

speza commented Apr 27, 2020

Agreed @Skarlso :) Thank you, will do.
And ha np. I will be honest, I don't favour the singleton approach in general 😛

@Skarlso
Copy link
Member

Skarlso commented Apr 27, 2020

@speza Oh definitely, I am not so happy about it either. But at that time I thought it's cool. Time passed, and I would like to think that I learned a lot since. :) So now I see better approaches which I would like to apply with providers.

But it's a lot of work and a massive refactor since these services are so deeply engrained. I'll get there in time... This is a first step. The scheduler was the smallest and resulted in a TON of change... :D

@speza
Copy link
Contributor Author

speza commented Apr 27, 2020

Yeah no worries @Skarlso! I also didn't mention anything before because I thought it was ok too :)

I'm a big fan of proper dependency injection wherever I write code these days, primarily Java still (ew) but been a lot more Go too recently. With Go I favour building the dependency higher up in the application and passing it all the way through where required. Still been meaning to try out something like https://github.com/google/wire - but in general I follow the pattern highlighted in their tutorial (just without the wire generated code).

# Conflicts:
#	frontend/client/views/settings/index.vue
#	frontend/src/views/permissions/index.vue
#	frontend/src/views/permissions/permissions/manage-groups.vue
#	frontend/src/views/permissions/permissions/manage-permissions.vue
#	frontend/src/views/permissions/permissions/permission-tables.vue
#	handlers/handler.go
#	handlers/permission.go
#	handlers/user.go
#	handlers/user_test.go
#	store/store.go
@speza
Copy link
Contributor Author

speza commented May 8, 2020

Closing. Working on a better solution :)

@speza speza closed this May 8, 2020
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.

[Permissions] Add the ability to define permission groups
3 participants