Migrate TUMOnlineRequest to Retrofit #940

thellmund · 2018-06-30T08:09:59Z

This is work in progress

This pull request replaces TUMOnlineRequest with TUMOnlineClient, which uses Retrofit. The steps we took:

We replaced SimpleXML with TikXML due to improved performance, smaller memory footprint and cleaner API.
We added TUMOnlineClient and TUMOnlineAPIService for API access.
We added TUMOnlineInterceptor to intercept requests, append the token parameter and throw exceptions in case of errors. This is necessary because TUMonline always returns 200, even in case of errors.
We removed TumLock and its associated DAO.

Building on these changes, we should do these things next:

Update string resources with more appropriate naming.
Update design of error layouts (nicer icons, larger text, proper headers).
Add TypeConverters (e.g. convert date strings to DateTime objects).

Issues: #544 #674 #388

It offers a cleaner API and is much faster than SimpleXML.

Reasons: 1. Cleaner API 2. Much faster 3. SimpleXML is no longer being developed

…o th/migrate-tumonlinerequest-to-retrofit

kordianbruck · 2018-07-20T20:19:53Z

@thellmund still WIP or Ready to review?

thellmund · 2018-07-20T20:43:58Z

@kordianbruck Basically done, but I want to do some more testing before submitting it for review. Will likely do that tomorrow.

…o th/migrate-tumonlinerequest-to-retrofit

…hub.com/TCA-Team/TumCampusApp into th/migrate-tumonlinerequest-to-retrofit

thellmund · 2018-07-21T13:46:32Z

@kordianbruck Ready for review

pfent · 2018-07-21T15:57:38Z

app/src/main/java/de/tum/in/tumcampusapp/api/app/Helper.java

@@ -80,7 +80,7 @@ public static OkHttpClient getOkHttpClient(Context c) {
        builder.connectTimeout(Helper.HTTP_TIMEOUT, TimeUnit.MILLISECONDS);
        builder.readTimeout(Helper.HTTP_TIMEOUT, TimeUnit.MILLISECONDS);

-        builder.addNetworkInterceptor(new TumHttpLoggingInterceptor(message -> Utils.logwithTag(TAG, message)));
+        //builder.addNetworkInterceptor(new TumHttpLoggingInterceptor(message -> Utils.logwithTag(TAG, message)));


please remove commented code

Commented this for testing. Will re-enable.

pfent · 2018-07-21T15:59:28Z

app/src/main/java/de/tum/in/tumcampusapp/api/tumonline/AccessTokenManager.java

     */
-    public boolean hasValidAccessToken() {
+    public static boolean hasValidAccessToken(Context context) {
        final String oldAccessToken = Utils.getSetting(context, Const.ACCESS_TOKEN, "");
        return oldAccessToken != null && oldAccessToken.length() > 2;


This is the only remaining method in AccessTokenManager?
Probably completely remove the class in favor of Utils or something

pfent · 2018-07-21T16:07:01Z

app/src/main/java/de/tum/in/tumcampusapp/api/tumonline/TUMOnlineClient.kt

+
+class TUMOnlineClient(private val apiService: TUMOnlineAPIService) {
+
+    fun getCalendar(force: Boolean = false): Call<Events> {


I really don't like boolean parameters.
From looking at a function call, one can't tell WTH this means.

getCalendar(true);

Without looking at the function definition, that parameter could be anything.
Please consider a proper Enum as parameter, which might even directly reference the underlying String.

getCalendar(CacheControl.doNotCache)

Is much nicer to read

Android Studio displays the parameter name, but I agree with your feedback. Will adapt this accordingly.

pfent · 2018-07-21T16:14:44Z

app/src/main/java/de/tum/in/tumcampusapp/api/tumonline/interceptors/CachingHelper.kt

+
+    companion object {
+
+        private val cachingDurations = mapOf(


When we're not actually using the Map capabilities of that map, because we always linearly search the map, please use a List of tuples instead of a Map.

pfent · 2018-07-21T16:15:20Z

app/src/main/java/de/tum/in/tumcampusapp/api/tumonline/interceptors/CheckErrorInterceptor.kt

+            TokenLimitReachedException::class,
+            InvalidTokenException::class,
+            MissingPermissionException::class,
+            UnknownErrorException::class)


If only there was a common exception type to be declared 🤔

pfent · 2018-07-21T16:41:55Z

...c/main/java/de/tum/in/tumcampusapp/component/tumui/lectures/adapter/LecturesListAdapter.java

-            holder.tvTypeSWSSemester.setText(String.format("%s - %s - %s SWS", lvItem.getStp_lv_art_name(), lvItem.getSemester_id(), lvItem.getDauer_info()));
-            holder.tvDozent.setText(lvItem.getVortragende_mitwirkende());
+            holder.tvLectureName.setText(lvItem.getTitle());
+            holder.tvTypeSWSSemester.setText(String.format("%s - %s - %s SWS",


pfent · 2018-07-21T16:42:26Z

app/src/main/java/de/tum/in/tumcampusapp/component/tumui/lectures/model/Lecture.kt

+        @PropertyElement(name = "sj_name") val semesterYears: String,
+        @PropertyElement(name = "stp_lv_art_kurz") val shortLectureType: String,
+        @PropertyElement(name = "stp_lv_art_name") val lectureType: String,
+        @PropertyElement(name = "stp_lv_nr") val stp_lv_nr: String,   // TODO: Rename valiables


you still want to do this?

At least fix the typo in the TODO 🙈

pfent · 2018-07-21T16:43:10Z

app/src/main/java/de/tum/in/tumcampusapp/component/tumui/lectures/model/LectureDetails.kt

+        @PropertyElement(name = "sj_name") val semesterYears: String? = null,
+        @PropertyElement(name = "stp_lv_art_kurz") val shortLectureType: String? = null,
+        @PropertyElement(name = "stp_lv_art_name") val lectureType: String? = null,
+        @PropertyElement(name = "stp_lv_nr") val stp_lv_nr: String,  // TODO: Rename variables


Leftover TODO

pfent · 2018-07-21T16:43:14Z

app/src/main/java/de/tum/in/tumcampusapp/component/tumui/lectures/model/LectureDetails.kt

+@Xml(name = "row")
+data class LectureDetails(
+        @PropertyElement(name = "dauer_info") val duration: String? = null,
+        @PropertyElement(name = "ersttermin") val firstAppointment: String? = null, // TODO: TypeConverters


Leftover TODO

Upon further inspection, I saw that the value can also be “geplant” (“planned”). For that reason, I’ll remove the to-do.

pfent · 2018-07-21T16:45:59Z

app/src/main/java/de/tum/in/tumcampusapp/component/tumui/person/model/Employee.kt

-
-        return infoText + name + ' ' + surname
+        return arrayOf(title, name, surname)
+                .filterNotNull()


aren't you missing empty strings for titles then?

Upon further inspection, I don’t believe that the string representation of Employee is used anywhere (we use getNameWithTitle(Context)). I’ve removed it now.

Grades are sorted by date, with the newest grade at the top.

pfent · 2018-07-21T21:09:08Z

app/src/main/res/values-de/strings.xml

@@ -92,7 +83,7 @@
    <string name="licences_summary">Rechtliche Hinweise zu Lizenzen</string>

    <!-- Startup Wizard -->
-    <string name="token_not_enabled">Nicht angemeldet</string>
+    <string name="not_connected_to_tumonline">Nicht angemeldet</string>


This is not a proper translation of "Not connected"

I think “angemeldet” fits better than “verbunden”, but since we seem to use the latter throughout the app, I’ve changed this translation.

pfent · 2018-07-21T21:12:17Z

app/src/main/java/de/tum/in/tumcampusapp/utils/StringExt.kt

+        compareTo(other)
+    } else {
+        -1
+    }


I think this is quite literally compareValues

I only learned about compareValues() through your comment today – thanks btw, @pfent 😊
Given that I stopped using this extension with the last commit, I’ll remove the entire file.

pfent · 2018-07-21T21:14:28Z

app/src/main/java/de/tum/in/tumcampusapp/utils/CacheManager.kt

+    private fun syncPersonalLectures() {
+        TUMOnlineClient
+                .getInstance(context)
+                .getPersonalLectures(CacheControl.NO_CACHE)


It his correct?
We call this in fillCache(), but then set NO_CACHE?

Ok, I still think this is confusing...
This should probably be called BYPASS_CACHE

I changed the naming accordingly and changed the method to CacheControl.USE_CACHE, because we only want to refresh the content in the background if it’s past its max-age (same behavior as before the refactoring).

…o th/migrate-tumonlinerequest-to-retrofit

pfent

Let the merge conflicts begin!

done

thellmund and others added 29 commits June 22, 2018 12:21

Switch from SimpleXML to TikXML

e851108

It offers a cleaner API and is much faster than SimpleXML.

Add Retrofit classes and custom interceptor

2617cb3

Switch from SimpleXML to TikXML

ee5736a

Reasons: 1. Cleaner API 2. Much faster 3. SimpleXML is no longer being developed

Rename variables and replace default empty values with optionals

f29a22a

Replace var with val in XML classes

cbccfcf

Switch to TUMOnlineClient for calendar activities

5f5bc47

Switch to TUMOnlineClient for loading tuition fees

472dde2

improve interceptor

05f4e92

Add handleDownloadError method in ActivityForAccessingTumOnline

fbfce01

Make TuitionFeesActivity use handleDownloadError method

6945c39

Update TUMonline calls in calendar activities

2da92ac

Use TUMOnlineClient for downloading person details

30b134f

Use TUMOnlineClient for loading grades

e565448

Use TUMOnlineClient for loading lecture details and appointments

c46448c

Use TUMOnlineClient for loading personal lectures

ed569a6

Use TUMOnlineClient for person search

e32625d

Use TUMOnlineClient for chat rooms

81c0c68

Use TUMOnlineClient for uploading the public key

7fb31d4

Use TUMOnlineClient for loading ids in onboarding

4df521d

Update IdentityTest to work with TikXml instead of SimpleXml

c829c28

Remove RxJava remnant in TuitionFeeManager

23afcb6

Improve error handling for requests to TUMonline

a4a1cb1

Merge branch 'master' of https://github.com/TCA-Team/TumCampusApp int…

472b6c7

…o th/migrate-tumonlinerequest-to-retrofit

Remove usage of TUMOnlineRequest in ChatRoomController

830801b

Remove TUMOnlineRequest and related classes

71a8126

Remove TumLock and add database migration

62b0b82

Fix wrong database version numbers in migration

2e9fcce

Add onRefresh() stub on ActivityForAccessingTumOnline

cfa4742

Make minor code improvement in TUMOnlineInterceptor

0ed138b

thellmund added the Work in progress label Jun 30, 2018

Merge branch 'master' into th/migrate-tumonlinerequest-to-retrofit

87114ab

thellmund added 5 commits July 21, 2018 14:59

Make minor code and UI adjustments

f86578d

Make code adjustments

dbfa5be

Make adjustment related to TikXML

497bf7b

Merge branch 'master' of https://github.com/TCA-Team/TumCampusApp int…

2d1d486

…o th/migrate-tumonlinerequest-to-retrofit

Merge branch 'th/migrate-tumonlinerequest-to-retrofit' of https://git…

4567b81

…hub.com/TCA-Team/TumCampusApp into th/migrate-tumonlinerequest-to-retrofit

thellmund requested review from kordianbruck and pfent July 21, 2018 13:46

thellmund removed the Work in progress label Jul 21, 2018

pfent suggested changes Jul 21, 2018

View reviewed changes

thellmund added 3 commits July 21, 2018 20:59

Address code review feedback

ad5699e

Add German translations for new string resources

e37bd83

Improve sorting of grades

af51dd8

Grades are sorted by date, with the newest grade at the top.

pfent reviewed Jul 21, 2018

View reviewed changes

thellmund added 3 commits July 21, 2018 23:30

Address code review feedback

786101c

Improve naming of CacheControl enum

13d1a8c

Merge branch 'master' of https://github.com/TCA-Team/TumCampusApp int…

7aa0966

…o th/migrate-tumonlinerequest-to-retrofit

thellmund requested a review from pfent July 23, 2018 08:01

pfent approved these changes Jul 23, 2018

View reviewed changes

thellmund added 2 commits July 23, 2018 11:05

Merge branch 'master' into th/migrate-tumonlinerequest-to-retrofit

0ada364

Merge branch 'master' into th/migrate-tumonlinerequest-to-retrofit

93f889d

thellmund merged commit f023a70 into master Jul 23, 2018

kordianbruck removed the review label Jul 23, 2018

thellmund deleted the th/migrate-tumonlinerequest-to-retrofit branch July 23, 2018 09:44

thellmund mentioned this pull request Jul 26, 2018

Re-enable and fix calendar sync #1020

Merged

kordianbruck mentioned this pull request Aug 2, 2018

Migrate TUMonline requests to retrofit #791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate TUMOnlineRequest to Retrofit #940

Migrate TUMOnlineRequest to Retrofit #940

thellmund commented Jun 30, 2018 •

edited by kordianbruck

kordianbruck commented Jul 20, 2018

thellmund commented Jul 20, 2018

thellmund commented Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

pfent Jul 21, 2018

pfent Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018 •

edited

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent Jul 21, 2018

thellmund Jul 21, 2018

pfent left a comment


		class TUMOnlineClient(private val apiService: TUMOnlineAPIService) {

		fun getCalendar(force: Boolean = false): Call<Events> {

Migrate TUMOnlineRequest to Retrofit #940

Migrate TUMOnlineRequest to Retrofit #940

Conversation

thellmund commented Jun 30, 2018 • edited by kordianbruck

kordianbruck commented Jul 20, 2018

thellmund commented Jul 20, 2018

thellmund commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thellmund Jul 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfent left a comment

Choose a reason for hiding this comment

thellmund commented Jun 30, 2018 •

edited by kordianbruck

thellmund Jul 21, 2018 •

edited