Mage #1326, Mage #1252, and Mage #1356 #72
Conversation
|
URL improvements look to be a duplicate of #67. |
| @@ -308,15 +308,20 @@ class UserRepository @Inject constructor( | |||
| private suspend fun syncIcon(user: User) { | |||
| val path = "${MediaUtility.getUserIconDirectory(application)}/${user.id}.png" | |||
| val file = File(path) | |||
| if (file.exists()) { | |||
| /// At the moment, I can't see any code that regularly updates User.lastModified | |||
There was a problem hiding this comment.
For reference, lastUpdated is tracked by the server and updated when the application fetches a user.
https://github.com/ngageoint/mage-android/blob/master/mage/src/main/java/mil/nga/giat/mage/network/user/UserWithRoleTypeAdapter.kt#L52-L55
and
https://github.com/ngageoint/mage-android/blob/master/mage/src/main/java/mil/nga/giat/mage/network/user/UserWithRoleIdTypeAdapter.kt#L48-L51
Users should only be fetched when selecting an event. This flow happens after login when selecting an event or after changing events.
There was a problem hiding this comment.
Stepping through the flow, users are indeed fetched when an event is selected, but afterwards it does an additional fetch for the icons. This change is fundamentally correct for only fetching icons that are needed. That said, there is an inherent inefficiency in having users pulled every load despite having a local storage for this data, it really should be checking lastModified to see if a user is even needed to be pulled, assuming it already exists locally. The exact letter of the ticket I was given was simply to stop icons from being loaded every time, but there is a further updated needed to fix this over-pulling of information.
There was a problem hiding this comment.
My comment wasn’t meant to suggest that your change was fundamentally wrong. Rather, it was intended to provide some context, if helpful, regarding the added comments.
For example, you mention:
the fact that we happen to pull users in their entirety every time we do something...
Users should currently be pulled when an event is selected, either at login or when changing events after login. Is that not the case? This would be drastically different from pulling them every time we do something.
The client has no way of determining if a user's remote properties have changed without retrieving that information. While we do have local storage for the user, it could be stale.
This is incorrect. They are an improvement of that issue, because that change was flawed and didn't work. The URL was never actually updated. |
|
Partially. You did fix a bug here setting the processedUrl after dropping the trailing '/' https://github.com/ngageoint/mage-android/blob/feature/mage-1326/mage/src/main/java/mil/nga/giat/mage/login/ServerUrlViewModel.kt#L74 However there is a regression as you no longer use the processedURL To avoid this regression and a merge conflict later I would suggest merging master back into develop first. |
I've placed those values, was never made aware that develop was behind master. |
Typically I would not stack so many issues into a single branch, but this is a special occasion due to the newness of our procedures.
MAGE #1326 Testing Procedures:
This is essentially an android upgrade, I also believe that due to the newly required granularity of location settings that MAGE #977 is also solved by this.
Due to the broad nature of this change, running through the other test procedures should prove this.
MAGE #1356 Testing Procedures:
As it stands, before this update if you attempt to use a MAGE server URL with a slash at the end it will fail and tell you that the URL is invalid. This is silly, and the code for this change was already there, it just needed to actually be implemented.
Assuming you were able to successfully log in, then the issue is successful as it no longer fails with the trailing slash.
MAGE #1252 Testing Procedures
Harder to test, as it is difficult to see from the front end if something is loading multiple times, so a bit of extra scrutiny on the code for this one. That Said, an event should still be done for proper testing coverage.
The only note is that this may load slightly faster, although not anything that can be noticed at our small sample size. This issue can only truly be noticed, and the benefits seen from.