Update introduces bug on muscle list

[wuwei7]

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):

• Device: Samsung Galaxy S8
• OS: Android 10

[brodeurlv]

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

[ghost]

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

[ghost]

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]

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]

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

[ghost]

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]

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 <#220 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD65NDQBJZSJM62VO2V7BJTUAKIEFANCNFSM5DAQXDWA> .