Closed sinergatis closed 8 years ago
sinergatis
commented
8 years ago Updated the pull request (previous comment was before 00f2d3c), basically fixing the ODPS feeds (at least for the basic cases - another look might be needed), and as a result including them in the tests.
Also added another test for checking how the views handled invalid primary keys.
sinergatis
commented
8 years ago Went ahead and added a new variable, ALLOW_USER_EDIT, for allowing the administrator to decided if all logged-in users are allowed to edit books/authors, or if only staff should be able to do so. A part of me feels that this should better be implemented via the standard Django permissions system (or check if userena or guardian makes things easier), though, but hopefully this should work in the meantime.
aristippe
commented
8 years ago Awesome awesome! Thanks for all the work on this!
For your questions:
where do we stand in regards to userena user registration features? I'm not sure if you have taken them into account yet or have any opinion or intentions in that regard.
I haven't looked into signup yet since I wasn't really planning on using at the moment. At least for myself, I don't plan on it though invitations through some other app or a custom one I might implement. If you think it's a good idea, feel free to add it, perhaps with a variable (or setting in a server model?) for allow public signup (?), though I don't think either of us is planning on using it, so no need if you're not going to use it. It can possibly wait until someone needs it.
I'm quite tempted to take the chance to remove the TagGroup model and its related views #25 as well, as currently they are never populated (unless manually from the admin) and need to be rewritten anyway to work with taggit. If you could confirm me that the tag (ie. regular tags, not tag groups) functionality is working as expected (adding, changing, listing books with tag, etc), and cannot find any arguments in favor of keeping them, that would be great.
Tagging functions are indeed working well. Tag groups I'm not sure what they were originally meant for (semi-hierarchical tags?) and I think there is even a Pathagar issue asking if tag groups are still needed. As far as I can tell, they can be removed.
More tests are great! For the ALLOW_USER_EDIT setting, I agree some permissions system might be better. I'm not sure who might want allow all users to edit books, though with a permissions system, maybe any settings one wants would be possible. I'll also try to look into how django handles groups as well see if guardian does anything.
sinergatis
commented
8 years ago Thanks for looking into the questions! Me happy :yum:
I haven't looked into signup yet since I wasn't really planning on using at the moment. At least for myself, I don't plan on it though invitations through some other app or a custom one I might implement. If you think it's a good idea, feel free to add it, perhaps with a variable (or setting in a server model?) for allow public signup (?), though I don't think either of us is planning on using it, so no need if you're not going to use it. It can possibly wait until someone needs it.
No problem, I just wanted to make sure if they needed to be taken into account for this pull request. Putting some thought into it later (and probably just delegate all the work in userena as it seems it has support for quite a bunch of user registration scenarios, perhaps with some sensible default settings) sounds fine!
Tagging functions are indeed working well. Tag groups I'm not sure what they were originally meant for (semi-hierarchical tags?) and I think there is even a Pathagar issue asking if tag groups are still needed. As far as I can tell, they can be removed.
Great, thanks for the confirmation! I'll remove them and make a note on the taggit vs tagging issue, should we ever find the need for introducing them again.
sinergatis
commented
8 years ago And merging this one as well! :ocean:
Please be aware that some migrations are included with this pull request (due to the removal of TagGroup, adjustment of the ImageField, and some leftovers from previous changes) - probably a migrate command would be in order at your development machine.
This pull request, still in progress, is basically a first attempt at fixing #36, making sure that Pathagar can be deployed:
ALLOW_PUBLIC_ADD_BOOKS).Additionally, as per the latest comment on #36, a part of me probably wants to also provide the possibility of deploying as a private server, allowing logged in users to browse, but only admins to edit/add/delete (
not implemented at the momenthttps://github.com/aristippe/pathagar/pull/41#issuecomment-193335101).The main changes are:
ALLOW_PUBLIC_BROWSEvariable, and removedbooks/app_settings.pyas it was basically just copying the mainsettings.pyvariables without real benefits.books/decorators.pyfile which take into account those variables, and adjustedurls.pyaccordingly (replacinglogin_requiredwith the custom decorator wherever needed). Also, moved all the decorators from views to urls, for consistency.except for the feeds due to #40b53a221, andcommentsanduserena) for the three kinds of users (admin, regular users, anonymous), checking for the expected response.add_languageview, as it was not really reachable via the UI, which in turned allowed for the removal of a number of files (popup stuff, templates, js, ...) that were only used by that view.In addition to the questions at #36, I'd love if you could let me know:
userenauser registration features? I'm not sure if you have taken them into account yet or have any opinion or intentions in that regard.TagGroupmodel and its related views #25 as well, as currently they are never populated (unless manually from the admin) and need to be rewritten anyway to work withtaggit. If you could confirm me that the tag (ie. regular tags, not tag groups) functionality is working as expected (adding, changing, listing books with tag, etc), and cannot find any arguments in favor of keeping them, that would be great.