Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New permission for wiki space #217

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

Conversation

monolithonadmin
Copy link

@monolithonadmin monolithonadmin commented Feb 26, 2024

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

Copy link
Member

@BreadGenie BreadGenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print statements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing for everywhere you've used print statements

@monolithonadmin
Copy link
Author

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
Copy link
Author

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

@monolithonadmin
Copy link
Author

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

@lost-RD
Copy link

lost-RD commented May 24, 2024

Excited for this!

@monolithonadmin
Copy link
Author

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
Copy link
Contributor

blaggacao commented Sep 6, 2024

@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
Copy link
Author

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.

@blaggacao

This comment was marked as outdated.

@blaggacao

This comment was marked as outdated.

Copy link
Member

@BreadGenie BreadGenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 220 to 228
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
Copy link
Member

@BreadGenie BreadGenie Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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`""")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 334 to 359
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much nesting and the variables aren't descriptive.

@JeansReal
Copy link

Hello!

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

@Z4nzu
Copy link

Z4nzu commented Sep 7, 2024

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

Copy link
Member

@BreadGenie BreadGenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Z4nzu
Copy link

Z4nzu commented Sep 10, 2024

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
Copy link

Z4nzu commented Sep 10, 2024

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
Copy link
Member

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
Copy link

Z4nzu commented Sep 11, 2024

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.

Copy link
Contributor

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 160 to 156
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 53 to 54
if frappe.db.exists('Wiki Page Revision Item', {'wiki_page': self.name}):
frappe.db.delete('Wiki Page Revision Item', {'wiki_page': self.name})
Copy link
Contributor

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(...)

Copy link

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

Copy link
Contributor

@blaggacao blaggacao Sep 13, 2024

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.

Copy link

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -348 to +344
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)
Copy link
Contributor

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(...)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@blaggacao blaggacao Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

Z4nzu commented Sep 12, 2024

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

@blaggacao
Copy link
Contributor

@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
Copy link

Z4nzu commented Sep 13, 2024

@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 😉.

Copy link

@Z4nzu Z4nzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all fixes and changes are done

Z4nzu

This comment was marked as duplicate.

@Z4nzu
Copy link

Z4nzu commented Sep 22, 2024

Hello @blaggaca @BreadGenie, Could you please review and accept requested changes? As, I already did those changes.

@monolithonadmin
Copy link
Author

Hello everyone,

First of all, thank you for your support. I know everyone is busy, and I really appreciate your time. I'm writing because I'd like to push this through as soon as possible. You've convinced me not to give up, and our contribution is valuable for everyone. To ensure we make this happen, we’ve allocated extra resources, which is why @Z4nzu has committed a lot of effort to this project.

I believe we are very close to finishing, so I’m asking for some additional support. Please review the commits, test them, and provide feedback or go ahead and merge them.

Thank you again!

Copy link

@Z4nzu Z4nzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes are done

@AyshaHakeem
Copy link
Collaborator

Hi @Z4nzu , Could you please list the steps a user has to follow to get the permissions to work? I couldn't implement it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants