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

fix: Prevent Users From Deleting All Vehicles #276

Conversation

windrunner21
Copy link
Contributor

What it Does

  • Closes Prevent Users From Deleting All Vehicles #202
  • I have added control to check the number of active vehicles user has, and if user has only 1 vehicle left, they cannot delete the last vehicle. If they press delete or swipe they are greeted with an alert.

How I Tested

  • Run the application, open Settings tab
  • Deleted vehicle using swipe gesture.
  • Was greeted with an Alert.
  • Deleted vehicle pressing on delete swipe action button.
  • Was greeted with an Alert.

Notes

  • Delete functionality itself wasn't implemented yet. I had already 2 vehicles added. Because I couldn't delete them from Firebase, I upped the control counter to 2 for local tests. I pushed the code with control counter set to 1.

Screenshot

  • Adding scree
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2023-11-30.at.16.48.08.mp4

n recording:

@windrunner21
Copy link
Contributor Author

@mikaelacaron I have also added some localization options for Turkish and Russian languages for the Alert I have created. I know English, Azerbaijani, Russian and Turkish languages. Also have limited knowledge in Japanese and French. If any help is needed in localization part, feel free to ask as well!

@windrunner21
Copy link
Contributor Author

@mikaelacaron hey, just checking up on you 😁 anything you need?

@mikaelacaron
Copy link
Owner

@windrunner21 Thanks! Sorry I haven't had a chance to look at this yet! I'll get to it this weekend

@windrunner21
Copy link
Contributor Author

Sure, looking forward to your comments! Good luck in the meantime!

@Jakayus
Copy link
Sponsor Contributor

Jakayus commented Dec 9, 2023

@windrunner21 @mikaelacaron This is a suggestion, but I am wondering if we can improve UX with the alert message text as the user might question why we can't delete the last vehicle. I used ChatGPT to quickly brainstorm some message versions and these seemed like the best.

"The last vehicle can't be deleted. Please ensure at least one vehicle remains in your account."
"You must have at least one vehicle in your account. Please add a new vehicle before deleting this one."
"Oops! Looks like this is your only vehicle. To keep using our services, you need at least one vehicle in your account."

Those are just suggestions/ideas! I also don't want to add a bunch of last minute work to the PR, especially considering localization efforts needed.

@windrunner21
Copy link
Contributor Author

@Jakayus hey, I agree with you as I was under the same impression while writing this feature. However, the reason I didn't decide to mention it to @mikaelacaron is that I think this feature is temporary, and this restriction may be removed in the future. If not, then it's not a problem for me, let's decide on the improved prompt and I'll add it!

Good catch!

@Jakayus
Copy link
Sponsor Contributor

Jakayus commented Dec 9, 2023

@windrunner21 Thank you for the background!

I would say let's go with The last vehicle can't be deleted. Please add a new vehicle before removing this one. This already includes some of what you have previously, so the localization is already halfway there. It's more action oriented than reason oriented, not sure what we prefer for UX.

I think too if we want to use a fancier/friendlier prompt, perhaps @mikaelacaron can weigh in on specific wording. The above is sort of the quickest fix.

Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

Great work!!

I think this wording is best, I'm going to manually change it and push it to your branch

The last vehicle can't be deleted. Please add a new vehicle before removing this one.

@mikaelacaron
Copy link
Owner

However, the reason I didn't decide to mention it to @mikaelacaron is that I think this feature is temporary, and this restriction may be removed in the future.

All good! I'm totally fine with you (and anyone) bringing up any suggestions for how I've written any feature!

This issue, the reason for it right now is because if you try to add a maintenance event or odometer reading, without having a vehicle, the row only says "Select a vehicle" without anything else. And because the vehicle doesn't exist, the user would need to figure out that they have to go back to settings to add the vehicle.

Adding an onboarding flow #40 prevents this from happening (for this MVP), but the user can still delete vehicles, so the overall issue still exists. #202 prevents the user from deleting all vehicles so this isn't a problem

Later on after the 1st version is shipped I'm thinking of a longer term solution to this issue

@mikaelacaron
Copy link
Owner

I have also added some localization options for Turkish and Russian languages for the Alert I have created.

Oh also @windrunner21 when adding new features, don't worry about not being able to translate everything into all the different languages this app supports (if you do know a language the app supports, like Russian) you can go ahead and add the translation for the new string

If you would like to add a new language that isn't supported yet, you can comment on #7 what language you'd like to add, and then open a PR with only those changes

@mikaelacaron mikaelacaron merged commit 4a116b6 into mikaelacaron:dev Dec 11, 2023
1 of 2 checks passed
@windrunner21
Copy link
Contributor Author

This issue, the reason for it right now is because if you try to add a maintenance event or odometer reading, without having a vehicle, the row only says "Select a vehicle" without anything else. And because the vehicle doesn't exist, the user would need to figure out that they have to go back to settings to add the vehicle.

@mikaelacaron I think we can display "Add vehicle" button instead of "Select a vehicle" row in this user story, in order to make it clear for the end user to add their vehicle first. If you think that this solutions makes sense, I can work on it if you'd like.

@mikaelacaron
Copy link
Owner

@mikaelacaron I think we can display "Add vehicle" button instead of "Select a vehicle" row in this user story, in order to make it clear for the end user to add their vehicle first. If you think that this solutions makes sense, I can work on it if you'd like.

This doesn't make sense right now because the user will try to tap on it, and still nothing happens

If you want to see this, in your simulator do "erase all settings" and then run the app, and you'll see

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.

Prevent Users From Deleting All Vehicles
3 participants