New permission for wiki space #217

monolithonadmin · 2024-02-26T11:42:14Z

Based on your ideas and our needs, we have implemented the missing permission feature. @BreadGenie Please review it and indicate your acceptance. Thank you.

#60

BreadGenie

I haven't tested out if it function or not yet, but the code needs some changes.

BreadGenie · 2024-02-27T11:28:49Z

wiki/wiki/doctype/wiki_page/wiki_page.py

@@ -216,6 +216,23 @@ def calculate_toc_html(self, html):
 	def get_context(self, context):
 		self.verify_permission("read")
 		self.set_breadcrumbs(context)
+		ccc = frappe.db.sql(""" select user, allow, for_value,name from `tabUser Permission` where user=%s and allow='Wiki Space' """,(frappe.session.user))


Suggested change

ccc = frappe.db.sql(""" select user, allow, for_value,name from `tabUser Permission` where user=%s and allow='Wiki Space' """,(frappe.session.user))

ccc = frappe.db.get_value("User Permission", {"user": frappe.session.user, "allow": "Wiki Space"}, ["user", "allow", "for_value", "name"])

Try to use database api and query builder as much as possible instead of raw sql
Also use better variable names. ccc is very ambigous

you have to rewrite it everywhere you're using frappe.db.sql

BreadGenie · 2024-02-27T11:29:00Z

wiki/wiki/doctype/wiki_page/wiki_page.py

@@ -216,6 +216,23 @@ def calculate_toc_html(self, html):
 	def get_context(self, context):
 		self.verify_permission("read")
 		self.set_breadcrumbs(context)
+		ccc = frappe.db.sql(""" select user, allow, for_value,name from `tabUser Permission` where user=%s and allow='Wiki Space' """,(frappe.session.user))
+		print(self.name)


remove print statements

same thing for everywhere you've used print statements

monolithonadmin · 2024-03-05T12:12:49Z

Dear @BreadGenie

Hopefully, my teammate has fixed all the requested issues. Could you please take a look? Is it good to merge like this, or should we open a new PR? Thank you

monolithonadmin · 2024-03-14T10:41:13Z

@BreadGenie please do not forget to check our last commit. Thank you

monolithonadmin · 2024-03-21T09:37:53Z

Dear @BreadGenie , we pushed a new code, hopefully everything is fixed now. Please take a look! 7b7dcfb
Thank you for your support

lost-RD · 2024-05-24T04:04:57Z

Excited for this!

monolithonadmin · 2024-09-06T10:11:16Z

Dear @BreadGenie can we help you somehow? I think we have done our part, unfortunately, we are not as good as you. Hopefully, our work gives you some idea and you have time finally to finish and merge this to the next release/update.

thank you

blaggacao · 2024-09-06T10:30:04Z

@monolithonadmin have you tried piping your change through an AI review? *

There are a couple of (relatively) obvious code smells that the AI would certainly catch, probably increasing your chances of a merge.

I've a pending review, but I didn't submit it because I felt like I was pointing out the too obvious things. So I thought, better leave that to an AI, but never said so.

Also, it seems that some of the maintainers' comments have been left unattended ever since? Maybe it's a good moment to go back and revise and address them carefully so that @BreadGenie can potentially remove his merge block?

In my experience, the onus of getting code merged is regularly on the proponent and the maintainers' requests seem quite reasonable to me.

* if you havn't got a preferred solution yet: consider https://aider.chat with an Anthropic Claude Sonnet API key, 2USD of recharge should be largely enough to get this PR into shape.

monolithonadmin · 2024-09-06T10:38:30Z

Dear @blaggacao thank you for your nice words and tips. Unfortunately, we cant do this anymore. We pushed till we can, I'm just hoping it's enough.

BreadGenie

Hey @monolithonadmin took a look at the PR. It needs some more changes. If you aren't interested anymore I'll give it a shot after a while (no promises)

Note: We do appreciate your contribution. I didn't come back to this again since the last requested changes weren't made.

BreadGenie · 2024-09-06T11:44:48Z

wiki/wiki/doctype/wiki_page/wiki_page.py

+		if results:
+			match_found = False
+			for result in results:
+				wikiGroup = frappe.db.get_all("Wiki Group Item", {"parent": result['for_value']},["name","wiki_page"])
+				if wikiGroup:
+					for wk in wikiGroup:
+						if wk['wiki_page'] == self.name:
+							match_found = True
+							break


This is too much nesting. Please fix it. You can refer https://gist.github.com/18alantom/d9f0565c0f42d6a71311d4a3093a1331#refactor-nested-blocks-as-functions to refactor this.

BreadGenie · 2024-09-06T11:46:52Z

wiki/wiki/doctype/wiki_page/wiki_page.py

@@ -283,6 +303,7 @@ def get_context(self, context):

 	def get_items(self, sidebar_items):
 		topmost = frappe.get_value("Wiki Group Item", {"wiki_page": self.name}, ["parent"])
+		wikiSpace = frappe.db.sql("""select name from `tabWiki Space`""")


Like I said please don't use raw sql unless absolutely necessary.
Also wouldn't this fetch all wiki spaces? Do we need it?

Also try to use snake case for variables. ie wiki_space.

BreadGenie · 2024-09-06T11:48:12Z

wiki/wiki/doctype/wiki_page/wiki_page.py

+			if check:
+				for c in check:
+					if c and c['for_value'] and c['for_value'] == sidebar_item.parent:
+						wiki_page = frappe.get_doc("Wiki Page", sidebar_item.wiki_page)
+						if sidebar_item.parent_label not in sidebar:
+							sidebar[sidebar_item.parent_label] = [
+								{
+									"name": wiki_page.name,
+									"type": "Wiki Page",
+									"title": wiki_page.title,
+									"route": wiki_page.route,
+									"group_name": sidebar_item.parent_label,
+								}
+							]
+						else:
+							sidebar[sidebar_item.parent_label] += [
+								{
+									"name": wiki_page.name,
+									"type": "Wiki Page",
+									"title": wiki_page.title,
+									"route": wiki_page.route,
+									"group_name": sidebar_item.parent_label,
+								}
+							]
+					else:
+						pass


Too much nesting and the variables aren't descriptive.

JeansReal · 2024-09-06T12:38:29Z

Hello!

I’m interested in the feat
I will take on the PR and made the changes accordingly

Z4nzu · 2024-09-07T09:28:58Z

Hi @BreadGenie & @monolithonadmin,
I hope you're both doing well. I've refactored the previous commits to minimize raw SQL usage, opting for ORM wherever possible. Additionally, I've eliminated nested loops.

I look forward to your review and the potential merge of this PR.
Thank you!!

BreadGenie

Can you add a feature flag in Wiki Settings to enforce permissions? It should default to false.

Z4nzu · 2024-09-10T10:47:57Z

Hello @BreadGenie and @monolithonadmin,

In Commit ID 20d0e93, I added additional functionality to enhance permission handling in the WikiPage module of the Frappe Wiki app. During testing, I discovered some bugs and realized that administrators should have unrestricted access to all wiki pages without needing to go through permission checks. I've implemented this change along with the following updates:

Stricter permission checks for normal users to ensure they only access authorized Wiki pages.
Administrators now have full access to all Wiki pages without restriction.
Enhanced user feedback with more descriptive error messages when access is denied.

Thank you!!

Z4nzu · 2024-09-10T10:48:54Z

Can you add a feature flag in Wiki Settings to enforce permissions? It should default to false.

Hello @BreadGenie, Can you please explain a bit more your thoughts that will help me to quickly implement this for wiki?

Thank you!!

BreadGenie · 2024-09-10T11:03:09Z

Hello @BreadGenie, Can you please explain a bit more your thoughts that will help me to quickly implement this for wiki?

We don't want the permission to be applied by default for every user (say for a fresh installation of Wiki). Once someone enables this feature via Wiki Settings then only it should apply the permissions IMO.

Also I'm not sure about simply throwing an error if user doesn't have the necessary permission. Since the user might not see the error thrown and it might confuse them and they may think that something broke (take a look at what the user see if they don't have perms). This can be thought again later.

Z4nzu · 2024-09-11T14:55:44Z

Hello @BreadGenie @monolithonadmin, I am here with latest changes cab8d49

PR Title:
Enhance Wiki Page Visibility and Permissions Handling

PR Description:

Guest User Access:
- Implemented logic to show pages to Guest Users if the Allowed Guest setting is enabled.
Revision Handling:
- Adjusted revision logic to prevent errors when no previous revisions exist. Now, if no revisions are found, the page simply won't display any revisions.
Context Optimization:
- Consolidated context updates for improved performance and code readability.
Custom Permissions:
- Added a new setting in Wiki Settings (Enable Custom Permissions). By default, this setting is False. When enabled, it enforces custom permission checks for wiki pages.

blaggacao

The review got messed up a bit (from phone, but I think it's ok now), but from my side, mainly the small performance hit might just be reverted.

blaggacao · 2024-09-07T09:37:56Z

wiki/wiki/doctype/wiki_page/wiki_page.py

+		permitted = frappe.has_permission(self.doctype, permtype, self)
 		if permtype == "read" and self.allow_guest:
 			return True
-		permitted = frappe.has_permission(self.doctype, permtype, self)


This change may represent a potential performance regression for no obvious reason. has_permission is not the cheapest of methods to run on anonymous requests.

blaggacao · 2024-09-12T09:36:45Z

wiki/wiki/doctype/wiki_page/wiki_page.py

+		if frappe.db.exists('Wiki Page Revision Item', {'wiki_page': self.name}):
+			frappe.db.delete('Wiki Page Revision Item', {'wiki_page': self.name})


frappe.db.delete_if_exists(...)

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!!

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.

This is done!! Thank you @blaggacao

frappe/semgrep-rules#29

blaggacao · 2024-09-12T09:40:22Z

wiki/wiki/doctype/wiki_page/wiki_page.py

-		return frappe.get_doc("Wiki Page Revision", last_revision)
+		if frappe.db.exists('Wiki Page Revision', last_revision):
+			return frappe.get_doc("Wiki Page Revision", last_revision)


frappe.db.delete_if_exists(...)

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!!

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.

Z4nzu · 2024-09-12T17:14:05Z

Hello @BreadGenie, We look forward to hear from you. Thank you!!

blaggacao · 2024-09-13T04:24:28Z

@Z4nzu Apologies for the imprecisions in my review. I mentioned, I was on the phone, trying to gain your candid reading 😄

I was mainly looking at it from a performance point of view (just in the diffs I saw), as this is public facing code.

Granted, the overall impact might be minor or it might not have been obvious, but if we can avoid two consecutive database calls, why wouldn't we then?

Z4nzu · 2024-09-13T04:41:23Z

@Z4nzu Apologies for the imprecisions in my review. I mentioned, I was on the phone, trying to gain your candid reading 😄

I was mainly looking at it from a performance point of view (just in the diffs I saw), as this is public facing code.

Granted, the overall impact might be minor or it might not have been obvious, but if we can avoid two consecutive database calls, why wouldn't we then?

Hello, I understand your concern and I will fix it as soon as I can 😉.

Z4nzu

This all fixes and changes are done

new Permission for wiki space

c279ea6

BreadGenie requested changes Feb 27, 2024

View reviewed changes

Updates query with builder

87957da

remove print statement and also update query structure

7b7dcfb

This comment was marked as outdated.

Sign in to view

BreadGenie requested changes Sep 6, 2024

View reviewed changes

Z4nzu and others added 2 commits September 7, 2024 12:56

Merge branch 'master' into master

d8e75bc

[COMMIT] frappe#217 Optimised code for New permission for wiki space.

ba8d5f7

BreadGenie requested changes Sep 10, 2024

View reviewed changes

Enhance permission handling for WikiPage in Frappe Wiki app

20d0e93

[COMMIT] Enhance Wiki Page Visibility and Permissions Handling

cab8d49

blaggacao reviewed Sep 12, 2024

View reviewed changes

Improved code.

d63880e

Z4nzu reviewed Sep 18, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

	ccc = frappe.db.sql(""" select user, allow, for_value,name from `tabUser Permission` where user=%s and allow='Wiki Space' """,(frappe.session.user))
	ccc = frappe.db.get_value("User Permission", {"user": frappe.session.user, "allow": "Wiki Space"}, ["user", "allow", "for_value", "name"])

		if frappe.db.exists('Wiki Page Revision Item', {'wiki_page': self.name}):
		frappe.db.delete('Wiki Page Revision Item', {'wiki_page': self.name})

New permission for wiki space #217

Are you sure you want to change the base?

New permission for wiki space #217

Conversation

monolithonadmin commented Feb 26, 2024 • edited Loading

BreadGenie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monolithonadmin commented Mar 5, 2024

monolithonadmin commented Mar 14, 2024

monolithonadmin commented Mar 21, 2024

lost-RD commented May 24, 2024

monolithonadmin commented Sep 6, 2024

blaggacao commented Sep 6, 2024 • edited Loading

monolithonadmin commented Sep 6, 2024

This comment was marked as outdated.

This comment was marked as outdated.

BreadGenie left a comment

Choose a reason for hiding this comment

BreadGenie Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeansReal commented Sep 6, 2024

Z4nzu commented Sep 7, 2024

BreadGenie left a comment

Choose a reason for hiding this comment

Z4nzu commented Sep 10, 2024

Z4nzu commented Sep 10, 2024

BreadGenie commented Sep 10, 2024

Z4nzu commented Sep 11, 2024

blaggacao left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaggacao Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaggacao Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Z4nzu commented Sep 12, 2024

blaggacao commented Sep 13, 2024

Z4nzu commented Sep 13, 2024

Z4nzu left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

monolithonadmin commented Feb 26, 2024 •

edited

Loading

blaggacao commented Sep 6, 2024 •

edited

Loading

BreadGenie Sep 6, 2024 •

edited

Loading

blaggacao left a comment •

edited

Loading

blaggacao Sep 13, 2024 •

edited

Loading

blaggacao Sep 13, 2024 •

edited

Loading