Skip to content

add audit to records #24#28

Open
mous13 wants to merge 1 commit into
masterfrom
record-audits
Open

add audit to records #24#28
mous13 wants to merge 1 commit into
masterfrom
record-audits

Conversation

@mous13

@mous13 mous13 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@mous13
mous13 requested a review from jannes-io June 24, 2026 16:51

@jannes-io jannes-io left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To fix the test I think you may have to persist the user that's being created during the setup.

Like self::getContainer()->get(EntityManagerInterface::class)->persist($this->user);

I'm not sure tho, it's still using the crappy traits to create the user instead of a foundry factory.

EDIT: the trait uses CreateUserService which should call the repository->save on the user, which should call entitymanager->persist, so I have no idea why the tests are whining about it not being persisted..


public function getNameForAudit(): string
{
return $this->getText();

@jannes-io jannes-io Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I'm actually not sure about this.

In real units, the text is very often left blank, or in other cases, the text is being used to include like a "citation" or something, which are often very long which would totally break the table.

I think it's best to remove this function from the trait, and have each record implement the function itself. Then for:

  • award records we could show the award text,
  • qualification records the qualifications,
  • rank records the rank,
  • combat records and service records we can grab the text, but we should limit it to max 32 characters.

My apologies for misguiding you in the issue.

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.

2 participants