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

1080: Add start day #1090

Merged
merged 21 commits into from
Aug 23, 2023
Merged

1080: Add start day #1090

merged 21 commits into from
Aug 23, 2023

Conversation

f1sh1918
Copy link
Contributor

resolves #1080

… protobuf, adjust card content to show date start, add stillInvalidCheck
Copy link
Contributor

@sarahsporck sarahsporck 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 android emulator.

Copy link
Member

@michael-markl michael-markl left a comment

Choose a reason for hiding this comment

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

Imo, the start date must also be added to the DB table and checked in the backend.

specs/card.proto Show resolved Hide resolved
@michael-markl
Copy link
Member

michael-markl commented Aug 20, 2023

You mean because you could change the time/date on the local device to make the card valid?
Hm couldn't we just add general check if the device date/timestamp fits to the backend timestamp?
Instead of checking each timestamp we add @michael-markl

Not sure what you mean by "each timestamp we add". I don't think there will be yet another timestamp in the future regarding the validity period.

Also I think, we should make sure that the backend has the (ultimate) capability of checking whether a card is valid or not (simply by looking at the row in the DB table). (Might also be interesting to have a statistic like "Currently valid cards" which we wouldn't be able to accurately determine if we don't add it to the DB). More importantly (imo), the verifyCard endpoint should always be correct and, hence, should also reflect whether the validity period has already started.

Implementing some validity checks on the frontend (of the verifier's device) and some validity checks on the backend seems a bit arbitrary to me from a design perspective.

Copy link
Member

@michael-markl michael-markl left a comment

Choose a reason for hiding this comment

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

Haven't tested

@f1sh1918 f1sh1918 requested a review from michael-markl August 21, 2023 18:12
…st for card info utils isCardNotYetValid, add a check that expiry date is after start day
@f1sh1918 f1sh1918 force-pushed the 1080-add-start-date branch from 4510f4b to 2e276f0 Compare August 21, 2023 19:46
@sarahsporck sarahsporck self-requested a review August 22, 2023 11:53
Copy link
Contributor

@sarahsporck sarahsporck left a comment

Choose a reason for hiding this comment

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

Again works as expected. I like the additional tests and checks. Checking the time is sth. we seemingly have overseen completely before.

@f1sh1918 f1sh1918 requested a review from michael-markl August 22, 2023 14:55
Copy link
Member

@michael-markl michael-markl left a comment

Choose a reason for hiding this comment

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

Not tested

@f1sh1918 f1sh1918 force-pushed the 1080-add-start-date branch from 7eab2ae to 1dbae00 Compare August 22, 2023 22:54
Copy link
Contributor

@sarahsporck sarahsporck left a comment

Choose a reason for hiding this comment

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

I left some minor comments. Please rerequest review once you have updated the last changes, then I'll look over it (hopefully) one last time

administration/src/bp-modules/cards/AddCardForm.tsx Outdated Show resolved Hide resolved
administration/src/cards/CardBlueprint.ts Outdated Show resolved Hide resolved
@f1sh1918 f1sh1918 requested a review from sarahsporck August 23, 2023 09:31
Copy link
Contributor

@sarahsporck sarahsporck left a comment

Choose a reason for hiding this comment

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

👌🏼 nice work. works fine.

@f1sh1918 f1sh1918 merged commit 59dbdf3 into main Aug 23, 2023
@f1sh1918 f1sh1918 deleted the 1080-add-start-date branch August 23, 2023 13:51
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.

[Sozialpass] Add start date
3 participants