search

geeksforsocialchange / PlaceCal

Bring your community together
https://placecal.org
GNU Affero General Public License v3.0
17 stars 8 forks source link

Allow editor of partner to set service area #1721

Closed ivan-kocienski-gfsc closed 1 year ago

ivan-kocienski-gfsc commented 1 year ago

Acceptance Criteria

Description

When logged in as User 147 (i believe?) and visiting https://admin.placecal.org/partners/oshun-abi-dougill/edit the service area selector has no service areas

Notes

If a admin User has no Neighbourhoods then when they go to edit a Partner the Service Area selector is empty. When I think what we want is- if the logged in User is on a Partner they own then they can set any Neighbourhood.

Implementation Notes

So if a logged in admin is viewing a partner that they own, they can set service areas regardless of their neighbourhoods

kimadactyl commented 1 year ago

I think we should let people set service areas if they can edit a partner. How it is now presumably means partners can't edit their own service areas either - doens't really make sense to me.

https://gfsc.notion.site/Partnership-Admins-57027fb4d8a644e091f3a0ac70e17e0b

kimadactyl commented 1 year ago

To be clear I know why we got here just with hindsight it seems a bit daft to let people set their own address but not service area!

katjam commented 1 year ago

I believe this immediate problem has been resolved by giving the user a neighbourhood. To futureproof for exactly this situate, the simplest thing to do would be inform the partner that they need to be assigned a neighbourhood in order to use the system. https://discord.com/channels/534700660327841792/1078353998983209010/1080537767429283941

kimadactyl commented 1 year ago

The problem is she can see and edit all 200 or so partners in Manchester now - this seems bad and I think makes the system somewhat unusable? It's not "breaking" in the bug sense but it feels high prio to fix for me

kimadactyl commented 1 year ago

Also like I say - presumably partners can't currently set their own service areas

katjam commented 1 year ago

The problem is she can see and edit all 200 or so partners in Manchester now - this seems bad and I think makes the system somewhat unusable? It's not "breaking" in the bug sense but it feels high prio to fix for me

Ah OK, so you still feel like it it is critical?

kimadactyl commented 1 year ago

Yes - i feel pretty strongly that partnerships should only have to worry about their own partners and not interact with everything out there. Feels like a recipe for disaster esp as we don't have any kind of admin logging if people start doing silly things.

ivan-kocienski-gfsc commented 1 year ago

It might also be worth putting in some feedback on this issue explaining that people who cannot assign anything to this partner can't make that selection at all (sorry for the bad wording)

kimadactyl commented 1 year ago

Confirmed partners can't set their own service areas - got some more asks about this today, this ticket is prob highest prio when we get back on it

katjam commented 1 year ago

@ivan-kocienski-gfsc will add implementation notes around watching out for service area related permissions

r-ferrier commented 1 year ago

I Had a look at this but the last comment on it says @ivan-kocienski-gfsc has some implementation notes to add - could you add these for us Ivan?

kimadactyl commented 1 year ago

Hey this keeps getting deprioritised and im not sure why - this is a critical bug that's stopping whole categories of our current users from editing things. It's marked vvv / ee also which is about as good as it gets - I am not sure why we have these designations if they're not being used to priortise tickets!

Moving to top - I think this is the biggest outstanding user bug.

katjam commented 1 year ago

Sorry my fault on the priority. I thought we'd decided that we didn't want to do anything that those top ones flagged as urgent in here - to focus on the homepage. I think it was blocked before because we were waiting for Ivan to comment which was done in the main description, but not flagged. All good to be worked on now, I think!

kimadactyl commented 1 year ago

yeah - its ok. i think the labels aren't really working for us tho, i see urgent was removed from it but im not sure why.

we prob need to do a little bit of a rethink here - having labels, milestone order, milestones themselves, vvv/eee labels etc to priortise things with no real guide to what each means in practice is causing a lot of confusion. we prob need to strip it back and start again at some point.

katjam commented 1 year ago

If I remember, we removed that urgent when we kicked off the dev work on 15 March because we decided it wasn't any longer urgent... but not sure who was there for that meeting.

kimadactyl commented 1 year ago

Yeah I mean it's really urgent and also far more urgent than other stuff that has been done like say, fixing the URL params for the filter. I think we need some better mechaisms for prioritising this kind of thing than just who happens to be at a given meeting. Maybe some kind of "what is a critical ticket?" checklist?

katjam commented 1 year ago

It's probably not fair to pre-populate the retro but I'll add this to the list so we don't forget.

ivan-kocienski-gfsc commented 1 year ago

okay i think i need a clarification:

given a user on the edit partner page:

Address admins are users who have 'owned neighbourhoods' that contain the partners address.

When serving the edit page PC looks at the policy to see if they can even see the page which does a "check if root", "check if neighbourhood admin", "check if owner" test. The next step for us is when the controller pulls in the list of neighbourhoods the user can set here which calls the neighbourhood policy which allows all neighbourhoods if the user is root but otherwise only the users neighbourhoods if they have any (is a neighbourhood admin) otherwise they have no neighbourhoods returned. The page is then rendered. There are no further checks on the incoming data on the POST side of the update so what ever the user has populated in the service area selector is taken as the authoritative set of neighbourhoods to be applied to this partner.

The difficulty is when writing tests all these edge cases have to be mapped out precisely and that last case is problematic to define. If I am an address admin editing a partner and i have no neighbourhoods then i cannot see any neighbourhoods in the service area selector but when i go to save the partner then i am stripping any service areas that i lack from that partner. Also I can change the partners address to something I don't have permission to access and then that partner disappears for me.

katjam commented 1 year ago

image