Implement AddOdometerReading AppIntent

[OmarHegazy93]

What it Does

• Closes FEATURE - Make an App Intent for Adding an Odometer Reading with Shortcuts #222
  Implement shortcut for adding new odometer reading

What is implemented?

1. Vehicle now conforms to AppEntity protocol, hence, it should have an explicit id, which represents documentId under the hood
2. VehicleQuery is simply fetching vehicles from the server
3. DistanceUnit enum is used to make it more readable for selecting Distance unit
4. In AddOdometerReadingIntent, validation is applied to Distance, and if the vehicles are empty, then the user will not be able to finish the shortcut

How I Tested

Happy Scenario

1. Install the app
2. Make sure at least one vehicle is added in the app by going to Settings tab > Add Vehicle
3. Switch to Shortcuts app
4. Tab on the + on the top right corner
5. Tab on ⨁ Add Action
6. Select Apps tab
7. Tab on Basic Car
8. Tab on Add Odometer Reading
9. Add data for all fields and tab the play button on the bottom right corner
10. See the result

Unhappy Scenario

Senario1

1. Install the app
2. Make sure at least one vehicle is added in the app by going to Settings tab > Add Vehicle
3. Switch to Shortcuts app
4. Tab on the + on the top right corner
5. Tab on ⨁ Add Action
6. Select Apps tab
7. Tab on Basic Car
8. Tab on Add Odometer Reading
9. Try to set distance value to zero or negative value
10. See the result (user won't be able to proceed if the distance value is less than 1)

Senario2

1. Install the app
2. Make sure no vehicles are added
3. Switch to Shortcuts app
4. Tab on the + on the top right corner
5. Tab on ⨁ Add Action
6. Select Apps tab
7. Tab on Basic Car
8. Tab on Add Odometer Reading
9. Try to set distance value to zero or negative value
10. See the result (user won't be able to proceed because there are no vehicles)

[mikaelacaron]

Thank you for submitting this!
I have several comments about the implementation, and a few questions

[mikaelacaron]

I see why you need this, but I don't like having this here in this struct because it makes it impossible to test this, but also because now AddOdometerReadingIntent depends on AuthenticationViewModel and OdometerViewModel

I'm not sure what the alternative is, which kinda goes back to my question about when is this struct called?

[OmarHegazy93]

let me know if the previous answer addresses your question, but here the only way we can access the user id to retrieve the Vehicles is through AuthenticationViewModel, however, one alternative solution would be persisting the vehicles locally on disk so that we can access vehicles from inside OdometerViewModel, hence, no need for the AuthenticationViewModel dependency anymore nor calling an api to fetch the vehicles from the server.
Or maybe storing the UserID somewhere and make it accessible inside `OdometerViewModel, but this one wouldn't make sense to me

[mikaelacaron]

The documentID shouldn't be set from the id, because it would always be empty

this gets set properly when fetching from Firebase

[OmarHegazy93]

So there are a couple of things going on here:

1. Since Vehicle conforms now to AppEntity protocol, it must be identifiable, meaning it must have a non-optional id
2. The property documentID was previously named id and since it's defined using @DocumentID property wrapper, it must be an optional value, so by keeping it as id will give a compilation error indicating that Type Vehicle does not conform to protocol AppEntity, which must be non-optional as motioned in the aforementioned point
3. The documentID will correspond to the key "_id" when sending/retrieving vehicle from firebase as the CodingKeys enum indicates
4. The reason why documentID is set to private property is not to confuse the caller for having documentID and id so I wanted the caller to deal with only one value that refers to the id of the vehicle and under the hood, we pass this id to the documentID and when calling vehicle.id, this implicitly get the documentID

Hopefully that made it clear

[mikaelacaron]

@OmarHegazy93 please click "resolve" on all the issues that you've addressed

[OmarHegazy93]

Sorry @mikaelacaron , I re-requested the review and forgot to submit the comments 😄, you should be able to see them now

[mikaelacaron]

Just another comment, thanks for all the work on this! Can you also update this branch from change on dev

[mikaelacaron]

So from what I've seen in examples we'd use @Dependency for these dependencies in the EntityQuery and AppIntent

Like:
@Dependency private var authViewModel: AuthenticationViewModel as a struct level variable, then we use it in perform and fetchVehicles, rather than initializing it

Can you add that here and in the other places too

[mikaelacaron]

@OmarHegazy93

[mikaelacaron]

we reviewed this live during my livestream!

https://youtube.com/live/xhlq3p9gxDM?feature=share

we didn't finish it, but there's a few comments of things we are going to change!

[mikaelacaron]

let's add OdometerViewModel as a dependency too