Skip to content
This repository has been archived by the owner on Sep 12, 2018. It is now read-only.

MapManager (replaces PublicManager) #107

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

plepe
Copy link
Contributor

@plepe plepe commented Jan 9, 2016

I think the PublicManager is not an ideal solution. This pull request replaces PublicManager by a MapManager with a visible() filter which takes the request object as parameter. It adds additional filters, so that maps of the current user are also included. This also simplifies adding additional permissions.

- old Map.public behaviour replaced by Map.objects.visible(self.request)
@yohanboniface
Copy link
Member

Purpose sounds OK, but tests are broken :s
Also you may need to add more to tests to cover the new subtleties of the manager.

@plepe
Copy link
Contributor Author

plepe commented Jan 10, 2016

Yes, I forgot about the tests. Will do soon.

I kept thinking about this change, and now I'm not so sure if I should remove the PublicManager, because it might break other code (I don't know if there are other users of leaflet_storage). Maybe deprecate PublicManager and print a warning if it is being used?

Btw, there's a visible change: the front page and the search page now include maps which are non-public but visible by the current user.

Talking about visibilities: I think the "My Maps" page only shows maps where the current user is owner. Shouldn't this page also include maps where the current user is editor?

@yohanboniface
Copy link
Member

I kept thinking about this change, and now I'm not so sure if I should remove the PublicManager,
because it might break other code (I don't know if there are other users of leaflet_storage). Maybe
deprecate PublicManager and print a warning if it is being used?

I feel like we can make this change. This will only change the behaviour for logged in users, and it will only show more maps that this user is allowed to see, so that's fine.

Talking about visibilities: I think the "My Maps" page only shows maps where the current user is
owner. Shouldn't this page also include maps where the current user is editor?

Yes, but only if map edit_status allow editors (or everyone). I.e. not in the case where someone is editor of a map, but the edit_status only allow the owner to edit it.

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.

2 participants