search

brodeurlv / fastnfitness

FastNFitness Android app: Body, Cardio and Fitness tracking.
https://play.google.com/store/apps/details?id=com.easyfitness
BSD 3-Clause "New" or "Revised" License
270 stars 356 forks source link

Update introduces bug on muscle list #220

Closed wuwei7 closed 3 years ago

wuwei7 commented 3 years ago

Hello, I've just updated the app to version 0.20.2 and noticed the inclusion of "Glutes" introduced a bug. All the muscles I had previously selected for my exercises were replaced by the muscle immediately above it on the muscle list. For example, on "Squat" I had selected "Thighs" but now it changed to "Shoulders", which is the muscle immediately above it on the muscle list. The same thing happened to all my other exercises. Thank you. Brandon Campos

To Reproduce Steps to reproduce the behavior:

  1. Go to 'Exercises'
  2. Click on any exercise you've previously added which includes at least one muscle from the muscle list
  3. On the "Exercise" tab click on "Muscles"
  4. See error

Expected behavior Muscle should not have changed.

Smartphone (please complete the following information):

brodeurlv commented 3 years ago

@MatthewRFennell, is this something you could investigate? Let me know if you are available for this and if you need some support. Thanks.

ghost commented 3 years ago

Oh no, I'm sorry about this! I'll take a look tomorrow.

ghost commented 3 years ago

I've identified the issue. It relates to the way the _musclesArray is used in getMuscleNameFromId() and getMuscleIdFromName().

These two functions convert the name to an integer based on an alphabetical ordering. So, in English, Abdominals has id 0 in the database, Back has id 1 etc.

Because Glutes come before Hamstrings, Obliques etc., all ids after Glutes were increased by 1 by my commit. So the values in already existing databases were not kept in-sync with the UI update.

This is made more complicated by choosing different languages. In English, Abdominals come first, followed by Back. However, in French, it's Abdominals followed by Thighs.

This means that changing languages will also cause the database values to be misinterpreted, and the correct fix will be different for different languages. That is going to make fixing everyone's database hard.

In my view, we have two options.

  1. Leave it as it is, and do our best to communicate that the values will have changed in the most recent update. Exercises made before the update will be broken, and users will have to manually update any incorrect exercises themselves. Exercises since the update will be fine.
  2. Remove Glutes again. The consequence of this is that most exercises that have been tagged since the last update will be incorrect, however any prior exercises will once again be interpreted correctly. If we go for this option, I will also need to modify getMuscleNameFromId() and getMuscleIdFromName() to handle out-of-bounds accesses (since the number of exercises will have decreased by 1)

I'd be inclined to go for number 1 as I'm worried about breaking things further by attempting to rollback the change. But, I'd also be happy to implement 2. What do you think @brodeurlv?

Either way, I think my next task should be to refactor the way muscles are represented in the database, to be independent of their name. This will allow us to add muscles more easily, and have the added benefit of the app not breaking if you change language.

PS: I'm sorry @wuwei7 and anyone else for being impacted by this bug. I should have realised my change would have this effect. I hope it has not caused too much inconvenience.

brodeurlv commented 3 years ago

Hi @MatthewRFennell , that's a great investigation ! Thank you for this and for the details. It is cristal clear.

The option 1 is not feasible because we have no easy wait to communicate and the impact would be to big for the historical users. The option 2 is not futur proof. At some point we will need to add glutes and then we will fall in the same situation.

So to me,

  1. First, it is time to build a more robust solution that doesn't rely on the id in the array that is language dependant but rather on fixed IDs with const or enum.
  2. Then, we need to migrate the database in DatabaseHelper.java to convert current recorded muscles to musclesV2, with the fixed IDs.

So you can extract getMuscleNameFromId() and getMuscleIdFromName() and buildMusclesTable() into a migration class and create a new class for the musclesV2.

The only weakness of this solution is for people who creates muscles with 0.20.2 but I think the impact is acceptable. This is not the main feature. So the sooner you can fix this, the smaller is the time where people can create false records.

Thanks again for the investigation. Let me know if you have questions and also if you are available to fix this is the coming days. I will publish a new release as soon as possible after that.

Thanks.

ghost commented 3 years ago

I agree with this approach, I have a few hours now so will take a look and report back with any progress / questions.

ghost commented 3 years ago

As a quick update, I've made some progres, I'm hpoing to get this done by Monday evening. Let me know if that timeframe doesn't work out for you! I'll post here if there are any delays.

brodeurlv commented 3 years ago

Thanks for the update. The time-line is fine, don't worry! 😊 Thanks for taking care of this.

Le sam. 4 sept. 2021 à 23:50, Matthew Fennell @.***> a écrit :

As a quick update, I've made some progres, I'm hpoing to get this done by Monday evening. Let me know if that timeframe doesn't work out for you! I'll post here if there are any delays.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/brodeurlv/fastnfitness/issues/220#issuecomment-913045714, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD65NDQBJZSJM62VO2V7BJTUAKIEFANCNFSM5DAQXDWA .