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

fix: unable to change rank in security records console, feat: add security record delete button, secure deleting all records #8016

Conversation

BartDrown
Copy link
Contributor

@BartDrown BartDrown commented Jan 5, 2025

About the pull request

This pull request improves slightly security records console.
I've fixed changing rank, so players on CO or Highcom paygrade are now actually able to change it in records.

This pull request also add option for authorized personel (CO/Highcom) to delete single record from console.
There've been some problems regarding records before, when MPs added them, but were unable to delete them. Now they at least can ask CO or contact highcom to delete those.

I've also secured removing all security records function, cause it doesn't seem for me like everybody with access to console, should be able to access that.

Fixes CIC/MPs don't have access to change records #7986
I've tested this change with Command, Highcomm and MPs players, didn't see any issues.

Explain why it's good for the game

CO/Highcom should be able to change person's rank in security records.
CO/Highcom should be able to delete personal records.
Only CO/Highcom should be able to bulk delete all security records.
I did that change cause I've been affected by this many times, made some records and were not able to delete them or modify in reasonable way.
I'm aware that whole console needs major rework, but those changes should be present to at least make security records somewhat maintainable.

Testing Photographs and Procedure

Screenshots & Videos

image

image

image

Put screenshots and videos here with an empty line between the screenshots and the <details> tags.

Changelog

🆑
fix: unable to change rank in security records console
add: add security record delete button
add: secure deleting all records
/:cl:

feat: add record delete button, secure deleting all records

Fixes CIC/MPs don't have access to change records cmss13-devs#7986
@cmss13-ci cmss13-ci bot added Fix Fix one bug, make ten more Feature Feature coder badge labels Jan 5, 2025
@realforest2001
Copy link
Member

Aside from CO having the change rank button this is fine.

@BartDrown
Copy link
Contributor Author

Aside from CO having the change rank button this is fine.

Is there then any reason to even have rank field editable then? It'll probably confuse every player that it's clickable and will arise more bug reports like this one #7986.
Only viable solution I can think of right now, is putting "Change rank" as separate button below record, with checks for highcom paygrade. That would clearly put emphasis that it is something separate and might require higher permissions.

Comment on lines +377 to +378
temp = ""
temp += "Personal record deleted."
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
temp = ""
temp += "Personal record deleted."
temp = "Personal record deleted."

Comment on lines +366 to +367
temp = ""
temp += "Are you sure you wish to delete [active1.fields["name"]] record?<br>This action may result in irreversible data loss.<br>"
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
temp = ""
temp += "Are you sure you wish to delete [active1.fields["name"]] record?<br>This action may result in irreversible data loss.<br>"
temp = "Are you sure you wish to delete [active1.fields["name"]] record?<br>This action may result in irreversible data loss.<br>"

temp += "<a href='byond://?src=\ref[src];choice=Purge All Records'>Yes</a>"
temp += "<a href='byond://?src=\ref[src];choice=Clear Screen'>No</a>"
else
alert(usr, "You do not have the required rank to do this!")
Copy link
Member

Choose a reason for hiding this comment

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

tgui_alert

Comment on lines +352 to +353
temp = ""
temp += "Are you sure you wish to delete all Security records?<br>This action may result in irreversible data loss.<br>"
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
temp = ""
temp += "Are you sure you wish to delete all Security records?<br>This action may result in irreversible data loss.<br>"
temp = "Are you sure you wish to delete all Security records?<br>This action may result in irreversible data loss.<br>"

Comment on lines +596 to +598
if (id_card)
if ( (id_card.paygrade in GLOB.co_paygrades) || (id_card.paygrade in GLOB.uscm_highcom_paygrades))
return TRUE
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
if (id_card)
if ( (id_card.paygrade in GLOB.co_paygrades) || (id_card.paygrade in GLOB.uscm_highcom_paygrades))
return TRUE
if(id_card && (id_card.paygrade in GLOB.co_paygrades) || (id_card.paygrade in GLOB.uscm_highcom_paygrades))
return TRUE

@@ -564,3 +587,14 @@ What a mess.*/
/obj/structure/machinery/computer/secure_data/detective_computer
icon = 'icons/obj/structures/machinery/computer.dmi'
icon_state = "messyfiles"

/obj/structure/machinery/computer/secure_data/proc/can_perform_restricted_actions(mob/user)
if (ishuman(user))
Copy link
Member

Choose a reason for hiding this comment

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

early return

@hry-gh hry-gh marked this pull request as draft January 9, 2025 11:34
@BartDrown
Copy link
Contributor Author

Currently this whole merge request is going through rework, due to rewriting whole security records console to TGUI.
Requested changes from review will be included in new merge request, sorry for the fuss

@BartDrown
Copy link
Contributor Author

BartDrown commented Jan 20, 2025

Said changes are included in this PR:

https://github.com/cmss13-devs/cmss13/actions/runs/12876851743/job/35900464089?pr=8191

I'm closing this as scope of changes dramatically changed and they deserve another PR.

@BartDrown BartDrown closed this Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature coder badge Fix Fix one bug, make ten more
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants