brarcher / loyalty-card-locker

Stores your barcode-based store/loyalty cards on your phone
GNU General Public License v3.0
171 stars 29 forks source link

New feature: Favorite cards #305

Closed t351206 closed 5 years ago

t351206 commented 5 years ago

Description: If you have some favorite loyalty cards it is useful to have them as first items in the listview. Therefore I added the "star-unstar" feature. Cards in the LoyaltyCardViewActivity can be be starred in order to make them a favorite card.

Therefore a new SQLite table column was added where the starring status is saved.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

t351206@gmx.de

brarcher commented 5 years ago

Thanks for your interest in the project!

Are you still working on the change, or is it ready to review now? I've not looked at much of the code yet, but one quick point that caught my eye was tests. Can you also include robolectric tests to cover the (1) database change, (2) importing and exporting the new field (including what happens if the field is not present [backwards compatibility]), and (3) behavior changes in activities? There should be enough examples of tests in the project to build from.

brarcher commented 5 years ago

I see that the build on Travis-CI failed because a library could not be found:

Could not resolve com.android.tools.layoutlib:layoutlib-api:26.2.0.

Do you know where this library comes from? Does something else from the Android SDK need to be pulled down prior to the build?

https://github.com/brarcher/loyalty-card-locker/blob/master/.travis.yml

t351206 commented 5 years ago

I have no idea what where this library comes from ....

brarcher commented 5 years ago

Looking it up, it appears the library is available in the maven repository. You will need to add a dependency to the build.gradle file. Perhaps add the following:

compile group: 'com.android.tools.layoutlib', name: 'layoutlib-api', version: '26.2.0'
t351206 commented 5 years ago

I just added it to build.gradle and now I get the following java compiler error: Caused by: com.android.builder.dexing.DexArchiveBuilderException: Failed to process .gradle\caches\modules-2\files-2.1\com.google.guava\guava\23.0\c947004bb13d18182be60077ade044099e4f26f1\guava-23.0.jar Caused by: com.android.builder.dexing.DexArchiveBuilderException: Error while dexing. Caused by: com.android.tools.r8.CompilationFailedException: Compilation failed to complete Caused by: com.android.tools.r8.utils.AbortException: Error: Default interface methods are only supported starting with Android N (--min-api 24): java.util.concurrent.ScheduledFuture com.google.common.util.concurrent.ListeningScheduledExecutorService.schedule(java.lang.Runnable, long, java.util.concurrent.TimeUnit)

By the way I get warnings that compile and testcompile are obsolete and should be replaced and a lot the the used librariers in biuld.gradle are also old

Sorry, I am lost here - in Android Studio it was working fine without compile group: 'com.android.tools.layoutlib', name: 'layoutlib-api', version: '26.2.0'

t351206 commented 5 years ago

with Java 1.8 it works again (see https://github.com/kizitonwose/android-disposebag/issues/2) changes see below:

compileOptions {
    sourceCompatibility JavaVersion.VERSION_1_8
    targetCompatibility JavaVersion.VERSION_1_8
}
t351206 commented 5 years ago

I think testReleaseUnitTest failed, because the test classes are not adapted to the source code changes? But I do not know so much about this to fix these issues ....

brarcher commented 5 years ago

I just added it to build.gradle

It looks like that added the missing dependency, and no other missing dependences were found. Good.

Sorry, I am lost here - in Android Studio it was working fine

Android Studio may be keeping track of the new dependencies elsewhere, or is more forgiving. Official releases are built using gradle on a terminal (much like what Travis-CI is doing), and are not as forgiving. (:

I think testReleaseUnitTest failed

There are a number of tests for this application which use Robolectric. These test most of the application code that is testable outside of using an Android emulator or device. Because you changed some of the existing methods the tests will need to be updated as well. Additionally, the new feature being added needs separate tests. The build, test, and lint build steps will all need to pass before the changes can be accepted.

All the tests can be found under app/src/test/java/protect/card_locker. Android Studio should be able to run them, and they can be run from a terminal with

./gradlew test

To get started it may be good to checkout the project without your changes and run the tests. This way you can see them running and get an idea where they are. After that, try adding your changes one at a time and running the tests. As the tests break, you will know what needs to be updated in smaller chunks. At the end of updating the tests you should have the tests passing with your changes.

After the tests pass, new tests will need to be added to account for the new feature. If you would like advise on what new tests would be good and where to put them, poke me when you get to that point.

By the way I get warnings that compile and testcompile are obsolete and should be replaced and a lot the the used librariers in biuld.gradle are also old

The dependencies for the projects eventually have new releases. Sometimes they fix bugs or add features, and sometimes they introduce new bugs. The dependencies should probably be investigated, tested, and updated if possible. However that need not be your focus (unless you want to look into it).

Hope it helps. (:

t351206 commented 5 years ago

Thanks for the explanations- I will work on it!

t351206 commented 5 years ago

I will reopen, again, when I am succesful with the tests ...

t351206 commented 4 years ago

Hi again,

I fixed all the old issues. All tests are working and in addition some new tests were added. I will open a new pull request!