Data refresh by todesj · Pull Request #110 · Student-Lyf/RamLife

todesj · 2021-11-13T22:30:40Z

No description provided.

This reverts commit e476f91.

Levi-Lesches

First of all, you clearly understand the structure of a service and how to create one. Problem is, you don't really need to here. The root of the issue is that you're not accessing these timestamps for the sake of showing them in the app; you need them to manipulate other data (by refreshing them). Which means it shouldn't be its own service that your models call directly, but rather something that should be handled implicitly by the services layer, maybe in Database.init.

If you split your code into two parts, 1) deciding whether to update data, and 2) updating the data, you should only make part 1 implicit in Database.init. Remember that the user can always manually refresh the data in the UI, so you should make part 2 a separate function, perhaps a .update() on each HybridX.

All my comments in the code are either formatting or just an in-context explanation of the above.

Levi-Lesches · 2021-11-14T18:54:05Z

lib/src/models/data/schedule.dart

 		];
 		setToday();
 		notifyListeners();
+		await Services.instance.database.dataRefresh.setRefreshData("schedule");


Notice how you're using HybridCalendar here. That means you're not guaranteed to get data from the cloud. Which is a good thing: the on-device database is just as valid. The only place where you'd know where you got the data from would be either:

an explicit call to calendarDatabase.cloud.getSomething()

in the HybridCalendar itself, where you can see which database is chosen for each function

Try to figure out a better place to put this.

Levi-Lesches · 2021-11-14T18:54:10Z

lib/src/models/data/user.dart

 			for (final String scope in scopeStrings)
 				parseAdminScope(scope)
 		];
+		await Services.instance.database.dataRefresh.setRefreshData("user");


Levi-Lesches · 2021-11-14T18:54:36Z

lib/src/models/view/home.dart

 		try {
 			await Services.instance.database.calendar.signIn();
 			await Services.instance.database.sports.signIn();
-			await schedule.initCalendar();


Do you not need to initialize the calendar?

I call schedule.init() in .getRefreshData(). schedule.init() calls init calendar in it so it was initializing twice

Levi-Lesches · 2021-11-14T18:57:29Z

lib/src/models/view/home.dart

 			await Services.instance.database.calendar.signIn();
 			await Services.instance.database.sports.signIn();
-			await schedule.initCalendar();
+			await Services.instance.database.dataRefresh.getRefreshData();


Without even seeing what this does, you can probably tell it's not really named that well. The above functions do things, like signIn or initCalendar, but this function just gets something (the refresh data) and ignores the result. I'll leave more comments in that function, just keep this in mind. You always want to name your functions in the form of a question, like isX or hasX, or a command, like getX, setX, and initX.

Levi-Lesches · 2021-11-14T19:06:18Z

lib/src/services/databases/dataRefresh/hybrid.dart

+  @override
+  Future<void> signIn() async { }
+  @override
+  final CloudDataRefresh cloud = CloudDataRefresh();
+  @override
+  final LocalDataRefresh local = LocalDataRefresh();
+  @override
+  Future<void> getRefreshData() => cloud.getRefreshData();
+  @override
+  Future<void> setRefreshData(String data) => local.setRefreshData(data);


Suggested change

@override

Future<void> signIn() async { }

@override

final CloudDataRefresh cloud = CloudDataRefresh();

@override

final LocalDataRefresh local = LocalDataRefresh();

@override

Future<void> getRefreshData() => cloud.getRefreshData();

@override

Future<void> setRefreshData(String data) => local.setRefreshData(data);

@override

Future<void> signIn() async { }

@override

final CloudDataRefresh cloud = CloudDataRefresh();

@override

final LocalDataRefresh local = LocalDataRefresh();

@override

Future<void> getRefreshData() => cloud.getRefreshData();

@override

Future<void> setRefreshData(String data) => local.setRefreshData(data);

First of all, spacing.

Secondly, notice that you separate the cloud and local databases by splitting them into getRefreshData and setRefreshData. That's good design within HybridDataRefresh, since you care about having a difference between the two. However, the function that uses the HybridDataRefresh (like Database.init or Services.init) won't care, and will just want the data to come from the right places automatically.

That's the whole point of the HybridX interface. It's to compile the cloud and local databases into a single class that knows when to use which so that every other function can call HybridX.doSomething() without worrying. All this to say that you should have a function, maybe Future<void> update(String data), that 1) downloads the refresh timestamp from the cloud, 2) compares it with the local timestamp, 3) updates the data if needed, and 4) save the current timestamp to the local database.

You'll notice a few problems, namely:

if you download each refresh timestamp separately, you're essentially reading the same document many times for no reason

how would HybridDataRefresh know how to update, say, HybridCalendar if it determines it needs to update the calendar? Each HybridX should be separate from the others (modular), so it wouldn't make sense to give it direct access to every other database.

I think those two problems highlight the need for a better spot for this to go. Maybe you can give each HybridX a .update function, and in Database.init, you can download all the refresh timestamps at once and call any .update functions that are needed.

wait .getRefreshData() and .setRefreshData() do two different things not something for just one local and the other on the cloud. The thing is the way I currently designed it, I don't need the ability to set the dates in the cloud from the app (at least for now). Also when Database.init() is called isn't the user not signed in yet?

wait .getRefreshData() and .setRefreshData() do two different things not something for just one local and the other on the cloud.

Of course, but the point is they're targeted at the local vs the cloud databases, and whoever uses HybridDataRefresh has to know that, which defeats the point of a HybridX class that can make the local vs cloud choice by itself.

The thing is the way I currently designed it, I don't need the ability to set the dates in the cloud from the app

So then you don't need both .get and .set in your interface, because the interface dictates what the local, cloud, and hybrid databases should share in common.

Also when Database.init() is called isn't the user not signed in yet?

Right, there are two cases:

the user is signed in, and Database.init can access Firestore

the user isn't signed in, Database.init cannot access Firestore and Database.signIn has to instead.

This whole feature only applies to case 1. If the user isn't signed in yet, you don't have to "refresh" anything, all the data will be downloaded in signIn. So you need an if statement in Database.init to check if the user is signed in (see the Auth class) and if so, see if the data needs to be updated.

Levi-Lesches · 2021-11-14T19:17:28Z

lib/src/services/databases/dataRefresh/implementation.dart

+    if(DateTime.parse(map["user"]!).isAfter(Services.instance.prefs
+        .getRefreshData("user")!)||
+        Services.instance.prefs.getRefreshData("user")==null){
+        await Services.instance.database.user.signIn();
+        await Models.instance.user.init();
+        print(map["user"]);
+    }
+    if(DateTime.parse(map["schedule"]!).isAfter(Services.instance.prefs
+        .getRefreshData("schedule")!)){
+          await Models.instance.schedule.init();
+          print(map["schedule"]);
+    }


I noticed the condition isn't exactly the same between these two -- is that important?

You made SharedPreferences.getRefreshData return DateTime?, but you immediately use !. Maybe you can make it non-nullable with a helpful error message if the right key can't be found.

The keys passed to SharedPreferences are simple nouns like schedule and user. You can imagine some future confusion because it sounds like it actually holds the user/schedule data. Maybe clarify by making it last_update_schedule or user_last_update.

You don't want to sign in any services here, because you should already be fully signed in. You can't access Firestore unless you're already authenticated, so you can't possibly have gotten the data above and not be signed in yet.

You don't want to be touching any models here. Services are fully initialized before the models are even touched, and that's by design. Each XModel should get its data from a HybridX. If you want to change the data that the model will get, make sure to change it in the database, and the model will pick it up. That's also why the cloud and local databases are separated but collected together in the HybridDatabase -- so that the model can use the local data when appropriate, and assume that it's in-sync with the cloud data.

The repetitive structure and the deep coupling (ie, that CloudDataRefresh knows about HybridUser and HybridCalendar and depends on them to exist) show that you need to put this all somewhere else. See my above suggestions.

but for the User Model isnt .signIn where it actually downloads the data?

Oh you're right, I was confusing that with the idea of actually authenticating (maybe that means we need to rename the function, maybe like onSignIn to show it only happens after sign-in, or saveData to show that's all we're doing).

So yes, signIn would be the right function to call, but again, not from here.

I noticed the condition isn't exactly the same between these two -- is that important?

I just forgot to add it to the other one oops

So yes, signIn would be the right function to call, but again, not from here.

Why not from here?

You don't want to be touching any models here. Services are fully initialized before the models are even touched, and that's by design. Each XModel should get its data from a HybridX. If you want to change the data that the model will get, make sure to change it in the database, and the model will pick it up. That's also why the cloud and local databases are separated but collected together in the HybridDatabase -- so that the model can use the local data when appropriate, and assume that it's in-sync with the cloud data.

Also don't I need to reinitialize the model the properly store the data in the right places to be presented in the UI??

Why not from here?

Because you should be keeping the services separate, and this service is doing way too much work in other services. You're better off building this functionality directly into the other services so they can stay independent.

Also don't I need to reinitialize the model the properly store the data in the right places to be presented in the UI??

You don't re-initialize, because they haven't been initialized. The models assume that HybridX.getY() will always give the right data, and HybridX.getY assumes that the local database is up-to-date. Since the last assumption is wrong, all you need to do is just fill the local database with the updated cloud data and let RamLife handle the rest.

Levi-Lesches · 2021-11-14T19:18:42Z

lib/src/services/preferences.dart

 	late SharedPreferences _prefs;

+	///
+	static String refreshData(String data) => "refresh_$data";


I'd say to remove this function. It doesn't say getRefreshData, but it implies that it actually does so, when really all it does it return a string. You can probably just hard-code this string template in the actual function.

Suggested change

static String refreshData(String data) => "refresh_$data";

but if we eventually add another form of data that needs to be updated isn't it just easier to just leave this?

See how I have it in my suggestion below. The "refresh_$data" part is good, because it lets you use a variable to create a unique string. I'm just saying it doesn't need to be its own function, and you can stick it into the below function.

Levi-Lesches · 2021-11-14T19:19:47Z

lib/src/services/preferences.dart


+	/// Returns the last date the data was retrieved from the
+	/// database as [DateTime].
+	DateTime? getRefreshData(String data){


You say you return a DateTime?, but you never return null. As you saw above, that caused some confusion with the !. Instead of using ! below, handle the case where you get null and spit out an error message, and that way you can return a plain DateTime.

but it will be null the first time the user updates the data no?

Yep! So the ! will result in an error. That's why you should explicitly check for null and handle it properly. The ! is really a last resort, similar to dynamic in that you're telling Dart "I know this will lead to an error if it happens, but it will never happen". You shouldn't be using it to silence the errors, because those errors usually mean you need to handle the null case.

but I do handle it within the .getRefreshData() function

Well firstly, it's not a good idea to say you return something you don't actually ever return -- in this case, null. It's impossible for this function to ever return null. Second, you have a ! in this function, which means you don't actually handle the null case in the HybridDataRefresh service because it hits this ! before it can be returned.

Levi-Lesches · 2021-11-14T19:20:17Z

lib/src/services/preferences.dart

+		final String? lastUpdate = _prefs.getString(refreshData(data));
+				return DateTime.parse(lastUpdate!);


Suggested change

final String? lastUpdate = _prefs.getString(refreshData(data));

return DateTime.parse(lastUpdate!);

final String? lastUpdate = _prefs.getString(refreshData(data));

return DateTime.parse(lastUpdate!);

Indentation

Levi-Lesches · 2021-11-14T19:21:29Z

lib/src/services/preferences.dart

+	Future<void> setRefreshData(String data, DateTime value) async{
+		await _prefs.setString(refreshData(data), value.toString());
+	}


Something I figured out quite late is that you actually don't need async/await -- you can return the Future<void> given to you by _prefs.setString directly.

If you know you're always going to use DateTime.now, you might as well use it here.

You shouldn't name it data because it doesn't actually hold any data, just the name of the data. You shouldn't name it key either, since the actual key is refresh_X. `name seems to be better.

You might as well get rid of the above refreshData function and make the string directly.

Suggested change

Future<void> setRefreshData(String data, DateTime value) async{

await _prefs.setString(refreshData(data), value.toString());

}

Future<void> setRefreshData(String name) =>

_prefs.setString("refresh_$name", DateTime.now().toString());

todesj · 2021-11-14T20:04:13Z

First of all, you clearly understand the structure of a service and how to create one. Problem is, you don't really need to here. The root of the issue is that you're not accessing these timestamps for the sake of showing them in the app; you need them to manipulate other data (by refreshing them). Which means it shouldn't be its own service that your models call directly, but rather something that should be handled implicitly by the services layer, maybe in Database.init.

If you split your code into two parts, 1) deciding whether to update data, and 2) updating the data, you should only make part 1 implicit in Database.init. Remember that the user can always manually refresh the data in the UI, so you should make part 2 a separate function, perhaps a .update() on each HybridX.

All my comments in the code are either formatting or just an in-context explanation of the above.

OK I know that a service might be overkill here but it is really that bad if I use. a service instead so I don't have to change it?

Levi-Lesches · 2021-11-14T20:15:27Z

It's not really about overkill -- overkill is kinda the name of the game around organization. It's about separation. The reason we stopped getting these super-weird bugs is because the database is completely separate from the rest of the backend, which is completely separate from the fronted. Services are meant to help with that: each service gets a chance to init, and a Database is a special service that can also signIn. Each HybridDatabase is a special type of Database that can also manage two other Databases, a local and a cloud. But still, everything's separate. Just like services shouldn't interact with each other, you don't want databases interacting with each other either.

The way it's set up now, the refresh process is its own database that has control over every other database, which you don't really want. It's simpler to modify the definition of a DatabaseService to also include a function for updating, just like it already has functions for initialization and signing in. That way, each HybridDatabase can "keep its hands to itself" and only handle its own changes, while the master Database class can call the updates on each one. In that way it's like how each service has an init function but the master Services class goes to each service and calls them one-by-one.

CLAassistant · 2021-11-24T05:18:08Z

All committers have signed the CLA.

…tudent-Lyf#115)

Co-authored-by: todesj <70974284+todesj@users.noreply.github.com>

* Added schedule_search.dart * Added schedule search model * Update lib/src/models/view/schedule_search.dart Co-authored-by: Levi Lesches <levilesches@gmail.com> * Update lib/src/models/view/schedule_search.dart Co-authored-by: Levi Lesches <levilesches@gmail.com> * Fixes some formatting * Added docs and exported schedule_search.dart in lib/models.dart * Cleaned up schedule_search.dart * Cleaned up models.dart * Added Subject.id, PeriodData.name, PerioData.dayName * Update lib/src/data/schedule/subject.dart Co-authored-by: Levi Lesches <levilesches@gmail.com> * Add Period.dayName, and Period.name (Student-Lyf#81) * Converted firestore/lib/src/helpers to firestore-py/lib/utils * Ported firestore/lib/src/services to firestore-py/lib/services * Removed (and revoked) Firebase credentials * Cleanup * Cleaned up logging/Firebase issues * Added some __init__.py * Ported firebase/firestore/node/calendar.dart to firebase/firestore-py/bin/calendar.py * Ported firebase/firestore/node/admins.dart to firebase/firestore-py/bin/admins.py * Ported firebase/firestore/node/feedback.dart to firebase/firestore-py/bin/feedback.py * Added some data python files and sections python files * Continued translating more files (Note: some are incomplete!) lib/data/schedule lib/sections/reader edited lib/utils/dir to use constants from constants.yaml * Added bin/faculty, bin/secions, facultylogic and facultyreader python files Also added constants.py to get data from constants.yaml * Ported firebase/firestore/node/students.dart --> firebase/firestore-py/bin/students.py * Cleanup * Delete student.py * Fix students.py and uploading * Added some more python files. many variable should be converted to snake_case * Changed camel case variables to snake_case * small edits, opted for a different constants.py file too * Fixed some typos * Fixed bug where testers had no schedule (instead of a blank one) * --- * Finished scripts to upload data * Changed dayNames to a list rather than set * Make Fridays a Friday schedule by default * Modified feedback.py to actually output feedback * Added students reader * Cleanup * Fixed a bug in faculty/logic.py where a (Student-Lyf#101) teacher's periods weren't being added to theit schedule, instead the teacher's schedule was being set to contain only one period. Co-authored-by: todesj <josht2004@gmail.com> Co-authored-by: todesj <70974284+todesj@users.noreply.github.com> Co-authored-by: Brayden Kohler <brayk0hler@gmail.com> Co-authored-by: BraydenKO <58378705+BraydenKO@users.noreply.github.com>

* Enabled Emulator use * Fixed emulator error * Update lib/src/services/firebase_core.dart Added doc comment for `FirebaseCore.shouldUseEmulator` Co-authored-by: Levi Lesches <levilesches@gmail.com>

Makes the schedule icon appear in the dashboard when the side sheet is not persistently open

Adds Podfile.lock to the repo and updates XCode.

Levi-Lesches · 2022-01-14T21:44:56Z

I cleaned up the commits by force-pushing. Some reviews are gone now but now it's good.

Levi-Lesches added 30 commits October 8, 2020 01:59

Removed Letters from pages library

1cf2ac1

Cleanup

013d6c2

Fixed some text labels

27ec58d

Removed references to Letters from the code

c8bce1c

Fixed ClassList bug

2ecfcfa

Merge branch 'master' into web

3bd66d3

Released version 2.0.1+3

ae0ce00

Testing new Podfile to speed up iOS build time

6aefa54

Added dayNames to user profiles

e54b84c

Hid Sports page from master app

afb1eb9

Fixed reload logic

f2fc0d3

Set default Special to covid

3ff53e1

Automatically remove outdated reminders

8e0143d

Switched PeriodReminderTime to number period, not based on Special

cc3dc50

Merge branch 'master' into web

0f1d8de

Cleanup

3d88b20

Merge branch 'master' into web

f2adee1

Fixed faculty Firestore bug

9d719da

Created constants.testers

d5d371c

Added redundancy error checking

0da5a6b

Migrated to new Crashlytics SDK (Android)

d470a2d

Changed name to RamLife (iOS, Android)

daedce7

Migrated to new Android Embedding (v2)

42e4ec3

Made HomePage stateless

7e7830c

Cleaned Student.getPeriods

9918f25

Documented data library

b9b8de8

Organized Services into Services and Databases

6aba893

Fixed models to handle new Service and Database

a8df7ee

Fixed pages to handle new Service and Database

99d5bc6

Disable Crashlytics during debug mode

dfc2f97

todesj and others added 4 commits November 5, 2021 15:30

Android auth (Student-Lyf#106)

5fb1bd6

Changed from the MIT license to the GPL v3.0

e476f91

Revert "Changed from the MIT license to the GPL v3.0"

8f9cbef

This reverts commit e476f91.

Added script to sign out all users

8377067

todesj requested a review from BraydenKO as a code owner November 13, 2021 22:30

Levi-Lesches reviewed Nov 14, 2021

View reviewed changes

todesj and others added 8 commits January 8, 2022 18:12

Fixes bug where users refreshed page but period time did not update (S…

fbee891

…tudent-Lyf#115)

Tap for Schedule Touch Ups (Student-Lyf#114)

00fec0e

Co-authored-by: todesj <70974284+todesj@users.noreply.github.com>

New urls (Student-Lyf#111)

10c9bbb

Enabled support for Firbease Local Emulator Suite (Student-Lyf#116)

433e2d5

* Enabled Emulator use * Fixed emulator error * Update lib/src/services/firebase_core.dart Added doc comment for `FirebaseCore.shouldUseEmulator` Co-authored-by: Levi Lesches <levilesches@gmail.com>

Subject refresh (Student-Lyf#118)

6567137

Dashboard fix (Student-Lyf#119)

21f5bd5

Makes the schedule icon appear in the dashboard when the side sheet is not persistently open

Ios folder xcode changes (Student-Lyf#120)

d454518

Adds Podfile.lock to the repo and updates XCode.

Levi-Lesches force-pushed the data-refresh branch from 8cfb1f3 to ef39a02 Compare January 14, 2022 21:42

todesj added 7 commits January 15, 2022 21:34

Adds a dataRefresh service

5007ba3

Fixes error

8c77f0e

Added new line

f9beb79

Adds user.signIn() to dataRefresh

5070ebd

Moved dataRefresh to database.Init()

4ad9a4e

Added print statements for debug testing

8d00c46

updated version number

ebd72d9

Levi-Lesches force-pushed the data-refresh branch from 7e7b036 to ebd72d9 Compare January 16, 2022 02:37

BraydenKO force-pushed the master branch from 6d714d9 to 16558f1 Compare October 25, 2023 03:08

BraydenKO requested a review from DavidTarrab as a code owner October 25, 2023 03:08

BraydenKO force-pushed the master branch from 16558f1 to 268f74a Compare October 25, 2023 03:18

		final String? lastUpdate = _prefs.getString(refreshData(data));
		return DateTime.parse(lastUpdate!);

Conversation

todesj commented Nov 13, 2021

Uh oh!

Levi-Lesches left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todesj commented Nov 14, 2021

Uh oh!

Levi-Lesches commented Nov 14, 2021

Uh oh!

CLAassistant commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Levi-Lesches commented Jan 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Nov 24, 2021 •

edited

Loading