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.

Sign up for GitHub

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

Jump to bottom

chore: Goodbye little Data Importer 👋 #4781

Merged
merged 4 commits into from
Nov 16, 2023
Merged

chore: Goodbye little Data Importer 👋 #4781

merged 4 commits into from
Nov 16, 2023

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Nov 13, 2023

Hi everyone!

Now that 99% of the user base has migrated to the Flutter app, we can remove the process of migrating lists from old apps (Android/iOS).

In this PR, this is essentially a giant git rm.
I have checked the app works on Android/iOS/macOS with a clean installation and an already existing one.

So for me, it's time to say goodbye to this code.
On iOS, I think reduce a lot the size, as Realm is a big dependency.

@g123k g123k requested a review from a team as a code owner November 13, 2023 09:42
@g123k g123k self-assigned this Nov 13, 2023
@g123k g123k linked an issue Nov 13, 2023 that may be closed by this pull request
PlatformException: PlatformException(Exception encountered, read, javax.crypto.BadPaddingException: error:1e000065:C... #3484
Closed
@g123k g123k added db migrations Database migrations and removed 🍎 iOS iOS specific issues or PRs 🧪 Tests dependencies 🌐 l10n MacOS labels Nov 13, 2023
@g123k g123k linked an issue Nov 13, 2023 that may be closed by this pull request
Importing data from V1: still relevant? [Blocked on F-Droid] #3150
Closed
monsieurtanuki
monsieurtanuki reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @g123k!
Looks good to me, except something with go_router where you don't set the language anymore, and it does look suspicious. Any explanation?

packages/smooth_app/lib/pages/navigator/app_navigator.dart
Comment on lines -293 to -294
ProductQuery.setLanguage(context, userPreferences);
context.read<ProductPreferences>().refresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see the relationship with the current PR. We need a language, right, regardless of data migration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops I didn't see your comment.
The reason is just above:

// The migration requires the language to be set in the app!

Copy link
Contributor

Choose a reason for hiding this comment

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

The migration requires the language to be set in the app!

So does the app, so I hope that setLanguage in migration was redundant with a setLanguage in the app and that indeed we can get rid of setLanguage/migration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I've checked 1 year and a half ago and the code was placed elsewhere.
I will move the code.

@teolemon teolemon added 🧴 Open Beauty Facts Please see this proposal: https://github.com/openfoodfacts/smooth-app/discussions/1378 📸 Open Products Facts 🐾 Open Pet Food Facts labels Nov 13, 2023
@teolemon
Copy link
Member

Ok, for the migrations of the legacy Open Beauty Facts (Android, iOS), Pet Food (Android) and Products (Android), we'll do something rash, I believe.

@g123k
Copy link
Collaborator Author

g123k commented Nov 15, 2023

Ok, for the migrations of the legacy Open Beauty Facts (Android, iOS), Pet Food (Android) and Products (Android), we'll do something rash, I believe.

Actually, if we go for a single app, there is no migration possible

@teolemon
Copy link
Member

small conflict to solve.
Given the numbers, We will just probably put a screen in the F-Droid apps and the Play Store apps asking the users to uninstall.
Capture d’écran 2023-11-15 à 12 17 46
Capture d’écran 2023-11-15 à 12 18 39

teolemon
teolemon approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

Fine by me

@g123k g123k marked this pull request as draft November 15, 2023 16:48
@g123k
Copy link
Collaborator Author

g123k commented Nov 15, 2023

I have to move some code and re-test everything.
So I convert the PR to a draft temporary

@g123k g123k marked this pull request as ready for review November 16, 2023 07:17
@g123k
Copy link
Collaborator Author

g123k commented Nov 16, 2023

I have re-tested everything and it's OK.
(Actually, it was also without the change, but we're now sure, there won't be any unexpected regression)

@teolemon
Copy link
Member

You served many users well

@teolemon teolemon merged commit 9d03d4b into openfoodfacts:develop Nov 16, 2023
5 of 6 checks passed
@github-actions github-actions bot mentioned this pull request Nov 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki left review comments

@teolemon teolemon teolemon approved these changes

Assignees

@g123k g123k

Labels
db migrations Database migrations dependencies 🍎 iOS iOS specific issues or PRs 🌐 l10n MacOS 🧴 Open Beauty Facts Please see this proposal: https://github.com/openfoodfacts/smooth-app/discussions/1378 🐾 Open Pet Food Facts 📸 Open Products Facts 🧪 Tests tests
Projects
None yet
Milestone
No milestone
3 participants
@g123k @teolemon @monsieurtanuki