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

NFSD Access Overhaul #1459

Merged
merged 13 commits into from
Jul 7, 2024
Merged

Conversation

neuPanda
Copy link
Contributor

@neuPanda neuPanda commented Jun 1, 2024

About the PR

NFSD Access Overhaul
Fixed ship console refresh on id insert or removal
Added Access to VesselPrototype
Added Filtration to Ship List based off of Access
Added Bailiff and Sergeant Access levels
Added Access levels to all NFSD ship
Added appropriate access groups to NFSD roles

Why / Balance

In Preperation for incoming NFSD changes

How to test

Go to NFSD ship yard
using each of the NFSD ID cards swap back and fort to observe available ships
ensure that non NFSD has no access
buy an empress and then test again with the empress ships
verify the new permissions for each of the NFSD roles at the ID console

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

🆑

  • fix: ship console refresh on id insert or removal
  • add: Filtration to Ship List based off of Access
  • add: Bailiff and Sergeant Access levels

@dvir001
Copy link
Contributor

dvir001 commented Jun 1, 2024

Add // Frontier to every single added line to code block

@neuPanda neuPanda requested a review from dvir001 June 4, 2024 05:28
@github-actions github-actions bot added the S: Needs Review This PR is awaiting reviews label Jun 4, 2024
@MagnusCrowe
Copy link
Contributor

MagnusCrowe commented Jun 11, 2024

image
SR has every access except Bailiff and Sergeant?! Sheriff has more access than the Station Representative?!
Yet, SR still has Detective access? Please explain your reasoning.

The ships Sheriff can access:

  • All from the NFSD shipyard
  • All from the Empress shipyard

The ships the Bailiff can access:

  • All but detective ships from the NFSD shipyard?! Explain.
  • All from the Empress shipyard

The Sergeant can access:

  • Prowler and Templar from the NFSD shipyard...no Hospitalier?
  • All from the Empress shipyard

The Deputy can access:

  • Nothing from the NFSD shipyard?! Not even a Templar? Explain.
  • All from the Empress shipyard

The Brigmedic can access:

  • Nothing from the NFSD shipyard?! Not even a Hospitalier? Explain.
  • All from the Empress shipyard

I don't understand the reasoning for these distinctions and lack of distinctions. What is the end goal here?

The way I see NFSD ships, there are 3 categories (don't read too far into the category names, don't "erm, acktully" me with naval technicalities please):

  • Flagships [Empress, Wasp] (almost always 1 per shift)
  • Cruisers [Marauder, Prowler, Opportunity hehe] (usually 1 or 2 per shift)
  • Support [Templar, Hospitalier, Interceptor, Inquisitor, Broadhead, Cleric, Fighter, Rogue] (many per shift)

There are categories of NFSD personnel too:

  • Sheriff
  • Officers [Bailiff, Sergeant]
  • Squad [Deputies, Cadets, Brigmedics, Detectives, Prison Guards]

I would reorganize the access as such:

  • Sheriff [Flagships, Cruisers, Support]
  • Bailiff [Cruisers, Support]
  • Sergeant [Marauder, Prowler, Support]
  • Deputy [Templar, Fighter, Rogue]
  • Brigmedic [Hospitalier, Cleric]
  • Detective [Interceptor, Inquisitor, Broadhead]
  • Prison Guard [None, what if a prisoner got their ID, get ships from bailiff]
  • Cadet [None, you shouldn't be operating your own ship before graduating the academy]

With these access levels every role has access to the ships they would typically pilot. If the Sheriff orders any of these jobs to procure a specific ship related to their job, they have access to do that. Each commander has the ability to purchase ships on behalf of the jobs that would be commanded by them. Let me know what you thing and what your reasoning was behind the choices you made.

@MagnusCrowe
Copy link
Contributor

On a more technical note, merge in the latest changes and set up the broadhead with access levels.

@Cheackraze
Copy link
Member

I agree with the suggested access changes actually, especially if we are going to be trying to steer the brigmedic away from officer mentality stuff or regular deputy stuff

@MagnusCrowe
Copy link
Contributor

image
Bailiff not intended to have detective access? Fine with me just seemed strange and not sure if it was intended...
All of my intention questions could be cleared up by stating what your intended changes are in the PR's About section.

@neuPanda
Copy link
Contributor Author

Bailiff not intended to have detective access? Fine with me just seemed strange and not sure if it was intended... All of my intention questions could be cleared up by stating what your intended changes are in the PR's About section.

correct, the detective works directly for the sheriff not the bailiff.
the detective needs to be able to investigate not only external issues but internal ones as well and can not have the bailiff interfering with their work

@github-actions github-actions bot added the S: Merge Conflict This PR has conflicts that prevent merging label Jun 30, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added S: Merge Conflict This PR has conflicts that prevent merging and removed S: Merge Conflict This PR has conflicts that prevent merging labels Jul 1, 2024
Copy link
Contributor

github-actions bot commented Jul 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@neuPanda neuPanda force-pushed the NFSD-access-expansion branch from d68542e to 0820a6a Compare July 2, 2024 03:23
@neuPanda
Copy link
Contributor Author

neuPanda commented Jul 2, 2024

points at github
now see here you little shit stop adding merge conflicts that were not there moments ago or so help me byte arrays i will make you suffer

@neuPanda neuPanda force-pushed the NFSD-access-expansion branch from 0820a6a to a5ca068 Compare July 2, 2024 03:32
@github-actions github-actions bot removed the S: Merge Conflict This PR has conflicts that prevent merging label Jul 2, 2024
@neuPanda
Copy link
Contributor Author

neuPanda commented Jul 2, 2024

glairs at github

@neuPanda
Copy link
Contributor Author

neuPanda commented Jul 2, 2024

pulls out the throngler

Copy link
Contributor

@whatston3 whatston3 left a comment

Choose a reason for hiding this comment

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

Throwing my hat in on this one.

I've taken a look through the code and come up with a set of suggestions on whatston3:NFSD-access-expansion-tests-and-suggestions

There's a diff without the merge of the current head of master up here

In brief:

  • Removed LINQ from the shipyard console code, replaced it with iterators and if statements.
  • Added checks for the ship's access against a role's groups as well as their tags - the sheriff can now buy a ship despite not having any explicit access tags.
  • Removed the officer/executive/sheriff security groups: too small to warrant separate groups at the moment to my mind, easier to just state outright which jobs have which accesses there. Base NFSD one's great, cadet's marginal.
    • If the idea was to have all of the definitions in one file, I suppose that's fair, but you'd still need to know both who is in what group and what those accesses are. Not hugely convinced of the value.
  • Removed redundant accesses and groups from Sheriff, Detective
  • Updated to the current head of master (mostly for testing)

In my (admittedly brief) testing so far, this seems to behave well.

neuPanda and others added 4 commits July 6, 2024 10:31
Fixed ship console refresh on id insert or removal
Added Access to VesselPrototype
Added Filtration to Ship List based off of Access
Added Bailiff and Sergeant Access levels
Added Access levels to all NFSD ship
Added appropriate access groups to NFSD roles
Fixing issue where re-sharper removed un-used using statements per request.
Fixed ship console refresh on id insert or removal
Added Access to VesselPrototype
Added Filtration to Ship List based off of Access
Added Bailiff and Sergeant Access levels
Added Access levels to all NFSD ship
Added appropriate access groups to NFSD roles
neuPanda and others added 4 commits July 6, 2024 10:31
Fixing issue where re-sharper removed un-used using statements per request.
that VS so politly decided to fuck up and then hide the fact that the code would not compile by compiling old code like the little shit that it is.  thank god i dont have to be proffessional with my code comments here like i do at work because i ant shit talk VS or Git when i am working so i shall unleach all my rage in this commnet for the 2 of them (git and VS) for being dicks and wasting everybody's time
@neuPanda neuPanda force-pushed the NFSD-access-expansion branch from 4059dfb to 10c0a45 Compare July 6, 2024 14:32
@neuPanda neuPanda requested a review from whatston3 July 7, 2024 02:24
Copy link
Contributor

@whatston3 whatston3 left a comment

Choose a reason for hiding this comment

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

Happy with the state of this. Good feature.

@whatston3 whatston3 dismissed dvir001’s stale review July 7, 2024 13:15

Atmospherics and Engineering accesses were removed. Checked with dvir, seemed fine with the changes.

@whatston3 whatston3 merged commit 9cd2e22 into new-frontiers-14:master Jul 7, 2024
12 checks passed
FrontierATC added a commit that referenced this pull request Jul 7, 2024
@whatston3 whatston3 mentioned this pull request Jul 10, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# FTL Map-Shuttle Map - Shuttle S: Needs Review This PR is awaiting reviews YML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants