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

Changes 25: New Changes Class #6690

Merged
merged 4 commits into from
Sep 20, 2024
Merged

Conversation

bastianallgeier
Copy link
Member

@bastianallgeier bastianallgeier commented Sep 19, 2024

Description

This is replicating #6681

Summary of changes

  • New Kirby\Content\Changes class to track changed models in the site.txt
  • The Changes class is used in the Changes controller to track and untrack changed versions.

Changelog

Enhancements

  • New Kirby\Content\Changes class to track changed models in the site.txt

Docs

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add changes & docs to release notes draft in Notion

@bastianallgeier bastianallgeier changed the title V5/changes/25 changes class Changes 25: New Changes Class Sep 19, 2024
@distantnative
Copy link
Member

Another thought: we could also stores this in a dedicated file in the site/log/ root instead of the site content file.

@bastianallgeier
Copy link
Member Author

I thought about this as well. Storing it in the site.txt really isn't that great after all. I was also thinking about a new /site/accounts/users.txt

I like site/log, but my only concern is that it would be a new directory that needs write permissions.

@bastianallgeier bastianallgeier force-pushed the v5/changes/25-changes-class branch from 6d4f96a to 0f0b079 Compare September 20, 2024 10:41
@distantnative
Copy link
Member

I do like /site/accounts/users.txt - could be an issue if the directory is excluded from uploads/git... etc. though?

@bastianallgeier
Copy link
Member Author

That's basically why I still went for the site.txt after all. Each alternative solution has potential pitfalls that we need to think through. That's why I thought I really want to get the functionality done first and we can still change the location later.

I know that there are a lot of "let's think about this later" points at the moment, but I honestly feel like we won't ever get this done if we don't approach it more pragmatically right now.

@distantnative
Copy link
Member

Ok, let's leave it with the site.txt.

@bastianallgeier bastianallgeier force-pushed the v5/changes/25-changes-class branch from 0f0b079 to 027136f Compare September 20, 2024 13:52
@bastianallgeier
Copy link
Member Author

The test coverage report is again completely wrong. I have no idea why this is failing so often to even recognize the @Covers annotations. So annoying.

Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

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

Me neither. There are factual unit tests covering the methods marked as uncovered.

@bastianallgeier bastianallgeier merged commit 6b664e2 into v5/develop Sep 20, 2024
10 of 11 checks passed
@bastianallgeier bastianallgeier deleted the v5/changes/25-changes-class branch September 20, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants