New permission for wiki space #217
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,17 +50,24 @@ def on_update(self): | |
| update_index(self) | ||
|
|
||
| def on_trash(self): | ||
| if frappe.db.exists('Wiki Page Revision Item', {'wiki_page': self.name}): | ||
| frappe.db.delete('Wiki Page Revision Item', {'wiki_page': self.name}) | ||
|
|
||
| # Get names of revisions that are not referenced in `Wiki Page Revision Item` | ||
| revisions_to_delete = frappe.db.get_all( | ||
| "Wiki Page Revision", | ||
| filters={ | ||
| "name": ["not in", frappe.db.get_all( | ||
| "Wiki Page Revision Item", | ||
| fields=["distinct parent"] | ||
| )] | ||
| }, | ||
| pluck="name" | ||
| ) | ||
|
|
||
| if revisions_to_delete: | ||
| frappe.db.delete("Wiki Page Revision", {"name": ["in", revisions_to_delete]}) | ||
|
|
||
| for name in frappe.get_all("Wiki Page Patch", {"wiki_page": self.name, "new": 0}, pluck="name"): | ||
| patch = frappe.get_doc("Wiki Page Patch", name) | ||
| try: | ||
|
|
@@ -72,8 +79,7 @@ def on_trash(self): | |
| for name in frappe.get_all("Wiki Page Patch", {"wiki_page": self.name, "new": 1}, pluck="name"): | ||
| frappe.db.set_value("Wiki Page Patch", name, "wiki_page", "") | ||
|
|
||
| frappe.db.delete("Wiki Group Item", {"wiki_page": self.name}) | ||
|
|
||
| clear_sidebar_cache() | ||
| remove_index(self) | ||
|
|
@@ -151,9 +157,9 @@ def update_page(self, title, content, edit_message, raised_by=None): | |
| self.save() | ||
|
|
||
| def verify_permission(self, permtype): | ||
| permitted = frappe.has_permission(self.doctype, permtype, self) | ||
| if permtype == "read" and self.allow_guest: | ||
| return True | ||
|
There was a problem hiding this comment. This change may represent a potential performance regression for no obvious reason. |
||
| if not permitted: | ||
| action = permtype | ||
| if action == "write": | ||
|
|
@@ -211,79 +217,80 @@ def get_context(self, context): | |
| self.set_breadcrumbs(context) | ||
|
|
||
| wiki_settings = frappe.get_single("Wiki Settings") | ||
|
|
||
| if sbool(wiki_settings.enable_custom_permissions) and frappe.session.user != "Administrator" and not self.allow_guest: | ||
| user_permissions = frappe.db.get_all("User Permission", filters={"user": frappe.session.user, "allow": "Wiki Space"}, pluck="for_value") | ||
|
|
||
| if not user_permissions or not frappe.db.exists("Wiki Group Item", {"parent": ["in", user_permissions], "wiki_page": self.name}): | ||
| raise frappe.PermissionError(_("It seems you don't have access to this page. Please contact an administrator if you believe this is a mistake.")) | ||
|
|
||
| wiki_space_name = frappe.get_value("Wiki Group Item", {"wiki_page": self.name}, "parent") | ||
| wiki_space = frappe.get_doc("Wiki Space", wiki_space_name) | ||
|
|
||
| # Fetch values that are used more than once | ||
| navbar_items = wiki_space.navbar_items or wiki_settings.navbar | ||
| light_mode_logo = wiki_space.light_mode_logo or wiki_settings.logo | ||
| dark_mode_logo = wiki_space.dark_mode_logo or wiki_settings.dark_mode_logo | ||
| revisions = frappe.db.get_all( | ||
| "Wiki Page Revision", | ||
| filters=[["wiki_page", "=", self.name]], | ||
| fields=["content", "creation", "owner", "name", "raised_by", "raised_by_username"], | ||
| ) | ||
|
|
||
| # Prepare context updates in a single dictionary | ||
| context_updates = { | ||
| "no_cache": 1, | ||
| "navbar_search": wiki_settings.add_search_bar, | ||
| "light_mode_logo": light_mode_logo, | ||
| "dark_mode_logo": dark_mode_logo, | ||
| "home_page": f"/{wiki_space.route}" if light_mode_logo or dark_mode_logo else None, | ||
| "script": wiki_settings.javascript, | ||
| "show_feedback": wiki_settings.enable_feedback, | ||
| "ask_for_contact_details": wiki_settings.ask_for_contact_details, | ||
| "wiki_search_scope": self.get_space_route(), | ||
| "metatags": { | ||
| "title": self.title, | ||
| "description": self.meta_description, | ||
| "keywords": self.meta_keywords, | ||
| "image": self.meta_image, | ||
| "og:image:width": "1200", | ||
| "og:image:height": "630", | ||
| }, | ||
| "edit_wiki_page": frappe.form_dict.get("editWiki"), | ||
| "new_wiki_page": frappe.form_dict.get("newWiki"), | ||
| "last_revision": self.get_last_revision(), | ||
| "number_of_revisions": frappe.db.count("Wiki Page Revision Item", {"wiki_page": self.name}), | ||
| "hide_on_sidebar": frappe.get_value("Wiki Group Item", {"wiki_page": self.name}, "hide_on_sidebar"), | ||
| "content": frappe.utils.md_to_html(self.content), | ||
| "page_toc_html": self.calculate_toc_html( | ||
| frappe.utils.md_to_html(self.content)) if wiki_settings.enable_table_of_contents else None, | ||
| "current_revision": revisions[0] if revisions else 0, | ||
| "previous_revision": revisions[1] if len(revisions) > 1 else {"content": "<h3>No Revisions</h3>", | ||
| "name": ""}, | ||
| "show_sidebar": True, | ||
| "hide_login": True, | ||
| "name": self.name, | ||
| "navbar_items": modify_header_footer_items(navbar_items), | ||
| "post_login": [ | ||
| {"label": _("My Account"), "url": "/me"}, | ||
| {"label": _("Logout"), "url": "/?cmd=web_logout"}, | ||
| {"label": _("Contributions ") + get_open_contributions(), "url": "/contributions"}, | ||
| {"label": _("My Drafts ") + get_open_drafts(), "url": "/drafts"}, | ||
| ], | ||
| } | ||
|
|
||
| # Handle conditional patch updates | ||
| if (context_updates["edit_wiki_page"] or context_updates["new_wiki_page"]) and frappe.form_dict.wikiPagePatch: | ||
| context_updates["patch_new_code"], context_updates["patch_new_title"] = frappe.db.get_value( | ||
| "Wiki Page Patch", frappe.form_dict.wikiPagePatch, ["new_code", "new_title"] | ||
| ) | ||
|
|
||
| # Apply all context updates in one go | ||
| context.update(context_updates) | ||
|
|
||
| def get_items(self, sidebar_items): | ||
| topmost = frappe.get_value("Wiki Group Item", {"wiki_page": self.name}, ["parent"]) | ||
| wikiSpace = frappe.get_all('Wiki Space', pluck='name') | ||
|
|
||
| sidebar_html = frappe.cache().hget("wiki_sidebar", topmost) | ||
| if not sidebar_html or frappe.conf.disable_website_cache or frappe.conf.developer_mode: | ||
|
|
@@ -295,6 +302,7 @@ def get_items(self, sidebar_items): | |
| context.current_route = self.route | ||
| context.collapse_sidebar_groups = wiki_settings.collapse_sidebar_groups | ||
| context.sidebar_items = sidebar_items | ||
| context.wikiSpace = wikiSpace | ||
| context.wiki_search_scope = self.get_space_route() | ||
| sidebar_html = frappe.render_template( | ||
| "wiki/wiki/doctype/wiki_page/templates/web_sidebar.html", context | ||
|
|
@@ -313,39 +321,27 @@ def get_sidebar_items(self): | |
|
|
||
| wiki_page = frappe.get_doc("Wiki Page", sidebar_item.wiki_page) | ||
|
|
||
| if wiki_page.allow_guest or frappe.has_permission(wiki_page.doctype, "read", wiki_page): | ||
| page_info = { | ||
| "name": wiki_page.name, | ||
| "type": "Wiki Page", | ||
| "title": wiki_page.title, | ||
| "route": wiki_page.route, | ||
| "group_name": sidebar_item.parent_label, | ||
| } | ||
| if sidebar_item.parent_label not in sidebar: | ||
| sidebar[sidebar_item.parent_label] = [page_info] | ||
| else: | ||
| sidebar[sidebar_item.parent_label].append(page_info) | ||
|
|
||
| return self.get_items(sidebar) | ||
|
|
||
| def get_last_revision(self): | ||
| last_revision = frappe.db.get_value( | ||
| "Wiki Page Revision Item", filters={"wiki_page": self.name}, fieldname="parent" | ||
| ) | ||
| if frappe.db.exists('Wiki Page Revision', last_revision): | ||
| return frappe.get_doc("Wiki Page Revision", last_revision) | ||
|
Comment on lines
-348
to
+342
There was a problem hiding this comment.
There was a problem hiding this comment. Hello @blaggacao, It looks like you have not analysed this code properly. I have added condition to return if exists as before it was throwing None not found error. I just handled this not deleting anything. Thank you!! There was a problem hiding this comment. Yeah, sorry, ignore that 😄 As I said, it came from the phone. If you want to be extra careful, in order to avoid two essentially unnecessary consecutive db calls, you could wrap it in a try except block. |
||
|
|
||
| def clone(self, original_space, new_space): | ||
| # used in after_insert of Wiki Page to resist create of Wiki Page Revision | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frappe.db.delete_if_exists(...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @blaggacao, I am not getting you why are you trying to make it complex? I checked but i have not found delete_if_exists method in develop branch nor in version-15 branch.
Please correct me if I'm wrong!!
Thank you!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/frappe/frappe/blob/develop/frappe%2F__init__.py#L1460
Sorry for the imprecise reference, here.
One db call is better than two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done!! Thank you @blaggacao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frappe/semgrep-rules#29