Stirling-PDF icon indicating copy to clipboard operation
Stirling-PDF copied to clipboard

Refactor permission variable names

Open balazs-szucs opened this issue 7 months ago • 6 comments

Refactor: Improve clarity of permission variable names

Renamed confusing can[Action] boolean variables to prevent[Action] (e.g., canPrint -> preventPrinting) in PasswordController.java, AddPasswordRequest.java, and add-password.html.

The previous can[Action] convention was misleading, as true meant the action was disallowed. The new prevent[Action] naming directly reflects the intent (true = prevented), improving code clarity.

Changes:

  • Updated variable names in controller logic
  • Updated @Schema descriptions in AddPasswordRequest.java
  • Updated corresponding HTML element attributes (id, name, for) in add-password.html

Important Notes:

  • The underlying logic still inverts the boolean when setting permissions (e.g., AccessPermission.setCanPrint(!preventPrinting)).
  • User-facing UI text remains unchanged per request of @Frooodle in #3420.

Why not invert the API logic

  • Inverting API (to can[action] logic) would either invalidate the UI
  • Inverting API AND changing UI would warrant bigger translation effort to change it in all languages
  • This version is consistent (meaning what the UI says is actually done) and preserve the UI language (meaning no translations needed) however it is inconsistent with PDFBox methods naming scheme

PDFBox

  • PDFBox Interaction: This refactor addresses the naming within Stirling-PDF's API and Front-end layers only. The controller logic intentionally inverts the prevent[Action] boolean (ap.setCanPrint(!preventPrinting)) to correctly interact with the underlying PDFBox methods. No further renaming related to these permissions is necessary as the PDFBox methods themselves retain the can[Action] names.

Underlying logic is not changed so it should work but just in case I tested locally on an Adobe PDF that contained form in Chrome.

New variable names in API

new API variable names

Related Issues:

Closes #3427 Closes #3420


Checklist

General

Documentation

UI Changes (if applicable)

  • [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR)

Testing (if applicable)

  • [x] I have tested my changes locally. Refer to the Testing Guide for more details.

balazs-szucs avatar May 01 '25 20:05 balazs-szucs

Thanks Ludy for the suggestion!

I went through the security API again, a bit more thoroughly and noticed the GetInfoOnPDF and AnalysisController also used the can[Action] so beside your suggestion I also went ahead and renamed those.

I think this should address everything relevant in the codebase.

balazs-szucs avatar May 02 '25 09:05 balazs-szucs

I just recently noticed that the get-info-on-pdf endpoint seem to broken. Strangely, my hosted instance (meaning mainstream version) of Stirling also seem be broken with my "setup". Can anyone verify if I am doing something wrong, or there is more going on. As for this pull the getPermissionState is inverted, so are the !ap.can[Action] so in theory atleast this pull request didn't actually change anything logic related (2 invertion = so the end result is the same NOT (NOT True) evaluates to NOT (False), which is True. NOT (NOT False) evaluates to NOT (True), which is False. ),

but further review and some investigation would be much appreciated.

balazs-szucs avatar May 02 '25 14:05 balazs-szucs

To elaborate on my findings: It seems no matter what PDF I feed to the get-info-on-pdf (and no matter what kind permission the PDF has) it always seem show the "default" setting which is "allowed".

Might be a problem with my setup and is actually unrelated to both the pull request and the codebase, but wanted to note that just in case.

balazs-szucs avatar May 02 '25 14:05 balazs-szucs

@Balazs-Szucs can you put the get info stuff into a new issue ticket and ill investigate

Frooodle avatar May 02 '25 14:05 Frooodle

@Balazs-Szucs can you put the get info stuff into a new issue ticket and ill investigate

Yes, @Frooodle. My apologies for the convoluted pull request, as for get info stuff it was just me testing locally, so it still entirely possible to be on my end, but I'll make mental note to raise a separate issue for it.

balazs-szucs avatar May 02 '25 15:05 balazs-szucs

Hi!

I am wondering if it would be helpful if delete the changes from get-info on PDF, and the changes that were address the "original" issue so to speak

Original issue being the API is misleading due to the fact that canPrint = true means you can't print, and my changes related to that was to rename stuff to reflect this behavior (e.g say preventX instead of canX). The get on pdf might needn't be addressed here in this pull request, and instead somewhere where the whole give more information/ explain operations do and summary page problem/enhancement is addressed explained here #2388 #3462

This was just an idea but I am open to suggestions :)

balazs-szucs avatar May 05 '25 10:05 balazs-szucs

Hi @Balazs-Szucs, yes, it would be great if you could move the changes for the get-info issue into another pull request so we can track that separately. Thanks for your help :)

DarioGii avatar May 09 '25 10:05 DarioGii

Hi @DarioGii

I reverted those changes. Sorry once again for the complicated pull request.

Thanks for your comment. If there is anything to change don't hesitate to comment :)

balazs-szucs avatar May 09 '25 12:05 balazs-szucs