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

1430: Extend card #1772

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

1430: Extend card #1772

wants to merge 8 commits into from

Conversation

seluianova
Copy link
Contributor

@seluianova seluianova commented Nov 13, 2024

Short description

Add the ability to extend the Koblenz Pass in the app.

  • The card is extendable if its expiry date is earlier than the endDate of the associated user entitlement
  • The user can see a notification 3 months (90 days) before the current card expires
  • When the user clicks the “Extend Card” button, he is redirected to the self-service portal

Design: https://www.figma.com/design/SDoGnXIjCCXpE1tAAJkRVc/entitlementcard?node-id=792-3416&t=kUofvHMqh07BbICq-1

image

Proposed changes

  • Adjust backend: add the 'extendable' field to the card verification result
  • Add ExtendCardNotification widget in the app

Not included:

  • In this PR an empty form to create card is opened (query params are not passed).
    Query params will be added in (or after) 1430: Add query params to card self service form #1749
  • Tests (separate tasks will be created)
  • I have not implemented a separate view for the case when the card has already expired, as suggested in the design, because I think our current view is fine, but feel free to object.

Side effects

  • not sure if german translations are correct
  • not tested on iOS

Testing

  1. Create and activate a KoblenzPass:
  • Open administration --> login as a koblenz project admin --> go to settings --> create token
  • Import user entitlements using curl or Postman
    curl -H "Authorization: Bearer <my_token>" -F file=@import_testdata_local.csv http://0.0.0.0:8000/users/import

CSV content:

regionKey,userHash,startDate,endDate,revoked
07111,"$argon2id$v=19$m=19456,t=2,p=1$cr3lP9IMUKNz4BLfPGlAOHq1z98G5/2tTbhDIko35tY",01.01.2024,01.01.2025,false
  1. Update endDate of the user entitlement to any future date using POST request or directly in the DB
  2. Open the app again and check the notification

Resolved issues

Part of: #1430

@seluianova seluianova linked an issue Nov 13, 2024 that may be closed by this pull request
@f1sh1918
Copy link
Contributor

f1sh1918 commented Nov 18, 2024

@hauf-toni
We should also provide a slightly adjusted message if the card was already expired, since the user may not open the app for 90 days
I also added some comments to the design, since i think there are some pieces missing

Edit: I see there is a design for that but it looks like its not implemented?

@seluianova
Copy link
Contributor Author

seluianova commented Nov 18, 2024

We should also provide a slightly adjusted message if the card was already expired

@f1sh1918 currently it looks like this, do you think it requires adjustment? (there was a small discussion in the task regarding that)
image

Edit: I see there is a design for that but it looks like its not implemented?

Yes, I wrote about that in the PR description

@f1sh1918
Copy link
Contributor

f1sh1918 commented Nov 18, 2024

We should also provide a slightly adjusted message if the card was already expired

@f1sh1918 currently it looks like this, do you think it requires adjustment? (there was a small discussion in the task regarding that) image

Hm so if you click on "Weitere Aktionen" and click on first Item the link will differ depending on the card can be extended or not?

i think this is missing also?
image

@seluianova
Copy link
Contributor Author

seluianova commented Nov 18, 2024

Hm so if you click on "Weitere Aktionen" and click on first Item the link will differ depending on the card can be extended or not?

That was the idea, yes. Keep the existing menu item (since we have it already, and it says ‘beatragen ODER verlängern’), just add query params if the card is extendable.

i think this is missing also?

It’s not missing, it was intentionally not changed. Because we already have a view for the case when the card has expired and I decided to keep it, to be consistent with other states when card is inactive (card revoked, card not yet valid, etc.)
Do you think it's better to be changed?

UPD. But I think I need to update the text for the expired card message, because for Koblenz there is no ‘Antrag für Verlängerung’ of course. Will do that.

@seluianova
Copy link
Contributor Author

When the pass expired, I adjusted the message to Ihr Pass ist abgelaufen. Unter "Weitere Aktionen" können Sie Ihren Pass verlängern oder einen neuen Pass beantragen. to make it suitable for when the card is expired AND extendable, and for when it is expired but NOT extendable.
I'm not sure if we need different messages (and views) for these two cases. But feel free to object.
image

@f1sh1918
Copy link
Contributor

f1sh1918 commented Nov 18, 2024

When the pass expired, I adjusted the message to Ihr Pass ist abgelaufen. Unter "Weitere Aktionen" können Sie Ihren Pass verlängern oder einen neuen Pass beantragen. to make it suitable for when the card is expired AND extendable, and for when it is expired but NOT extendable. I'm not sure if we need different messages (and views) for these two cases. But feel free to object. image

Alright thx. Gonna check your pr.
Sorry for not reading the whole pr description.
I always complain about that if other reviewers don't do that :/

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Looks really good actually. 👍

I couldn't finish the testing, will continue tomorrow. Just left you some comments

Update:
i think the extension container should have a box shadow too

You may use the flutter standard card widget for that which also brings lots of build in features
https://api.flutter.dev/flutter/material/Card-class.html

@seluianova
Copy link
Contributor Author

I think I have addressed all the comments, @f1sh1918 pls take a look

@seluianova
Copy link
Contributor Author

seluianova commented Nov 19, 2024

I removed my previous comment and added a minimal elevation to the notification to add some volume (I feel like the shadow shouldn't be the same as for the card...)
image
I also accidentally messed up the commits a bit and did rebase, sorry if you have to reset the branch locally because of that 🙄 (not sure)

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Works fine on ios 👍.
Just one small thing commented

If you don't want to wait until the url is final (your other pr) then please create another ticket and add a ticket nr to the todo

),
SizedBox(height: 8),
FilledButton(
style: FilledButton.styleFrom(
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 the style should be defined in themes.dart to be able to reuse it easily without adding any styles
Elevation and shape shouldn't change for this CTA-button.
For backgroundColor we can even define color primary as default. If we need a secondary action button we can just override the color then.

@seluianova
Copy link
Contributor Author

seluianova commented Nov 20, 2024

If you don't want to wait until the url is final (your other pr) then please create another ticket

@f1sh1918 I was not going to close the current task, untill everything is done. I find it convenient, but maybe we should discuss this topic in the team, if task splitting is more preferable in such cases 🙈

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.

Extend card
3 participants