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.

Sign up for GitHub

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

Jump to bottom

1840: Properly serialize activity log and fix it #1844

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

1840: Properly serialize activity log and fix it #1844

wants to merge 2 commits into from

Conversation

steffenkleinle
Copy link
Member

Short description

The activity log was broken for a while due to serializing not happening properly. This introduces proper typing and serializing.

Proposed changes

  • Add fromSerialized and serialize methods to each extension and for cards
  • Properly serialize activity log and use iso8601

Side effects

  • Old activity logs break the serializing (but this should not happen as they are only session based)

Testing

See issue.

Resolved issues

Fixes: #1840

@steffenkleinle
Copy link
Member Author

The additional fromSerialized and serialize methods for extensions are only necessary if user display strings are different form serialized values. Currently, this is only the case for dates (dd.MM.yyyy vs yyyy-MM-dd (ISO8601)). Obviously, we could also serialize dates as dd.MM.yyyy but I think in the long term this is better and provides more flexibility. Let me know if you disagree.

f1sh1918
f1sh1918 approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

General note: This should be deployed as a hotfix. so please do not merge anything from main

One small improvement. Rest looks good to me! 👍
Thx for the quick fix

Tested on chrome.
Also tested card creation for koblenz, nuernberg and bayern (also bulks)

administration/src/cards/extensions/AddressFieldExtensions.ts Show resolved Hide resolved
bahaaTuffaha
bahaaTuffaha approved these changes Dec 19, 2024
View reviewed changes
Copy link

@bahaaTuffaha bahaaTuffaha left a comment

Choose a reason for hiding this comment

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

Tested on Brave and Firefox. At nuernberg
It works great 👌
Interesting use of reduce at serializeCard/deserializeCard .

  • Unrelated point: BavariaCardTypeExtension is weird TBH are we inserting a component within an object?

@steffenkleinle
Copy link
Member Author 

* Unrelated point: BavariaCardTypeExtension is weird TBH are we inserting a component within an object?

Yes, kind of. We set a component as object property. In the end, a component is just a render function that returns a react element in any case. That way, we can have generic components for each extension (without needing to know anything about the actual implementation/look).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f1sh1918 f1sh1918 f1sh1918 approved these changes

@bahaaTuffaha bahaaTuffaha bahaaTuffaha approved these changes

@ztefanie ztefanie Awaiting requested review from ztefanie ztefanie is a code owner

@seluianova seluianova Awaiting requested review from seluianova seluianova is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix broken activity log
3 participants
@steffenkleinle @f1sh1918 @bahaaTuffaha