Skip to content

Refactor permission variable names #3457

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Balazs-Szucs
Copy link
Contributor

@Balazs-Szucs Balazs-Szucs commented May 1, 2025

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:

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)

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

@Copilot Copilot AI review requested due to automatic review settings May 1, 2025 20:28
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 1, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors permission variable names to improve clarity by renaming misleading boolean variables from the "can[Action]" convention to "prevent[Action]".

  • Updated HTML checkbox input attributes in add-password.html
  • Renamed variables and updated @Schema descriptions in AddPasswordRequest.java
  • Refactored variable names and inversion logic in PasswordController.java

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/main/resources/templates/security/add-password.html Updated checkbox IDs and names to use the new naming scheme
src/main/java/stirling/software/SPDF/model/api/security/AddPasswordRequest.java Refactored boolean variable names and schema annotations
src/main/java/stirling/software/SPDF/controller/api/security/PasswordController.java Renamed variable references and maintained inversion logic

@github-actions github-actions bot added Java Pull requests that update Java code Front End Issues or pull requests related to front-end development Back End Issues related to back-end development API API-related issues or pull requests labels May 1, 2025
Copy link
Contributor

@Ludy87 Ludy87 left a comment

Choose a reason for hiding this comment

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

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels May 2, 2025
@Balazs-Szucs
Copy link
Contributor Author

Balazs-Szucs commented May 2, 2025

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 Balazs-Szucs requested a review from Copilot May 2, 2025 12:44
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors permission variable names for clarity by renaming misleading boolean flags (from can[Action] to prevent[Action]) in both backend and frontend files. The key changes include updates to variable names and associated schema descriptions, inversion of booleans when interfacing with PDFBox, and renaming of HTML attribute ids and names.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/main/resources/templates/security/change-permissions.html Updated checkbox ids/names to use the new prevent[Action] convention
src/main/resources/templates/security/add-password.html Updated checkbox ids/names to match the new boolean naming scheme
src/main/java/stirling/software/SPDF/model/api/security/AddPasswordRequest.java Renamed fields and updated @Schema annotations for clarity
src/main/java/stirling/software/SPDF/controller/api/security/PasswordController.java Updated permission extraction and inversion to work with the new field names
src/main/java/stirling/software/SPDF/controller/api/security/GetInfoOnPDF.java Adjusted JSON output to reflect inverted permission states and new terminology
src/main/java/stirling/software/SPDF/controller/api/AnalysisController.java Updated JSON permission keys but with one naming inconsistency noted
Comments suppressed due to low confidence (1)

src/main/java/stirling/software/SPDF/controller/api/AnalysisController.java:182

  • The key name 'preventPrint' is inconsistent with the new naming convention used elsewhere (e.g., 'preventPrinting'). Consider renaming it for consistency.
permissions.put("preventPrint", !document.getCurrentAccessPermission().canPrint());

@Balazs-Szucs Balazs-Szucs requested a review from Copilot May 2, 2025 12:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR renames confusing permission variables to clarify their intent by replacing the misleading can[Action] names with prevent[Action] names, while preserving the underlying inverted logic.

  • Renamed variables and updated UI element attributes in HTML templates.
  • Adjusted API request classes and controller logic to reflect the new naming conventions.
  • Updated the permission state reporting in API controllers to align with the renaming.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/main/resources/templates/security/change-permissions.html Updated checkbox ids/names and corresponding label references for permission actions.
src/main/resources/templates/security/add-password.html Updated checkbox ids/names and corresponding label references for permission actions.
src/main/java/stirling/software/SPDF/model/api/security/AddPasswordRequest.java Revised Schema descriptions and variable names to improve clarity.
src/main/java/stirling/software/SPDF/controller/api/security/PasswordController.java Updated variable references and boolean inversions to match the new naming.
src/main/java/stirling/software/SPDF/controller/api/security/GetInfoOnPDF.java Adjusted boolean inversion when reporting permission states to reflect naming changes.
src/main/java/stirling/software/SPDF/controller/api/AnalysisController.java Revised permission mapping keys and boolean inversions in the security info API.
Comments suppressed due to low confidence (1)

src/main/java/stirling/software/SPDF/controller/api/AnalysisController.java:183

  • Ensure that the mapping of permission keys in AnalysisController (e.g., 'preventModify') is consistent with the new naming convention and with UI/API consumers' expectations across the system.
permissions.put("preventModify", !document.getCurrentAccessPermission().canModify());

@Balazs-Szucs
Copy link
Contributor Author

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

Balazs-Szucs commented May 2, 2025

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.

@Frooodle
Copy link
Member

Frooodle commented May 2, 2025

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

@Balazs-Szucs
Copy link
Contributor Author

@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 Balazs-Szucs changed the title Improve clarity of permission variable names Refactor permission variable names May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API-related issues or pull requests Back End Issues related to back-end development Front End Issues or pull requests related to front-end development Java Pull requests that update Java code size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: add-password API is logically inverted?
3 participants