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

Add Server Name to Logs and Enables the Searching of Them + Changelog #53

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Geekyhobo
Copy link
Contributor

@Geekyhobo Geekyhobo commented Jan 28, 2024

Server Searching and Server name now show up, this was way harder than originally thought to get working.
image
image

Plus we got a changelog on the settings page, and cleaned the settings page up a bit (updated cuase the first one sucked)
image

Also added Dapper a pretty sweet micro ORM
closes #41 and #9
Also should have made the logs page overall faster, it used to try and search the whole players catalog to find a null player every time the logs page was loaded, now it only does it if you search for a player

SS14.Admin/AdminLogs/AdminLogRepository.cs Outdated Show resolved Hide resolved
SS14.Admin/AdminLogs/AdminLogRepository.cs Outdated Show resolved Hide resolved

public class DapperDBContext : DbContext
{
public DapperDBContext(DbContextOptions options) : base(options)
Copy link
Member

Choose a reason for hiding this comment

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

Why are you defining a new database context like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Afaik that's because dapper can't use the same context to get the db connection as efcore

Copy link
Member

Choose a reason for hiding this comment

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

I've had no issues doing this myself when I've tried.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dapper tries to close the connection using the efcore context and that causes problems, ill give it another go to see if I can fix it

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't make sense, Dapper doesn't control connection lifetimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still pretty new to this whole thing so I'm gonna keep working on it

SS14.Admin/Pages/Settings/Index.cshtml Outdated Show resolved Hide resolved
{
await connection.OpenAsync();

var result = await connection.QueryAsync<AdminLog, string, WebAdminLog>(
Copy link
Member

Choose a reason for hiding this comment

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

JsonDocument instances for the logs need to be manually disposed so they can be pooled. I believed EF Core did it for us before, but now that's not an option you'll have to take care of it yourself somewhere.

SS14.Admin/Helpers/YmlHelper.cs Outdated Show resolved Hide resolved
SS14.Admin/Helpers/YmlHelper.cs Show resolved Hide resolved
SS14.Admin/SS14.Admin.csproj Outdated Show resolved Hide resolved
SS14.Admin/Pages/Settings/Index.cshtml Outdated Show resolved Hide resolved
SS14.Admin/changelog.yml Outdated Show resolved Hide resolved
@Geekyhobo
Copy link
Contributor Author

I made this PR less shit, fixed everything that was asked couldnt figure out how to use dapper without using a seperate context, please halp

public LogImpact Impact { get; set; }
public DateTime Date { get; set; }
public string Message { get; set; } = default!;
public JsonDocument Json { get; set; } = default!;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're still not disposing of the JsonDocument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i forgor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 Needs Review
Development

Successfully merging this pull request may close these issues.

Need a Changelog and Credits
3 participants