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.

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

New feature: Favorite cards #369

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

New feature: Favorite cards #369

wants to merge 25 commits into from

Conversation

t351206
Copy link

@t351206 t351206 commented Jul 19, 2020

Description (this is a follow-up to #305 ):

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.

[email protected]

Copy link
Contributor

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

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

Please note that I am not the owner of this project, so my opinion has less value than that of @brarcher, but I figured I would chime in anyway.

I also want to state that I'm missing tests for this starring functionality.

I do think the idea of the future itself is great though and I'm pretty sure I'll use it myself a lot too! Just the code could do with some cleaning up :)

Please note that @brarcher is currently on hiatus so it may take longer to get his opinion on this. I am willing to help you with parts of your pull request if you need any help. Thank you for making this :)

@t351206 t351206 marked this pull request as draft July 22, 2020 19:38
t351206 added 5 commits July 22, 2020 21:40
corrected CsvDatabaseImporter bugs

Signed-off-by: t351206 <[email protected]>
"starred" renamed to starStatus for better readability

Signed-off-by: t351206 <[email protected]>
@t351206
Copy link
Author

t351206 commented Jul 24, 2020

I also want to state that I'm missing tests for this starring functionality.
@TheLastProject Which test do you think would make sense?

@t351206 t351206 marked this pull request as ready for review July 24, 2020 15:56
@TheLastProject
Copy link
Contributor

I would like to see the following tests:

  1. UI shows correct buttons and buttons cause correct state change (once for starring and once for unstarring)
  2. Import CSV file with no star information
  3. Import with star information
  4. Import with invalid star information
  5. Main screen showing starred cards on top correctly
  6. Import from URI
  7. Import from URI with bad data

I think that should cover things

@t351206 t351206 marked this pull request as draft July 25, 2020 06:35
@t351206
Copy link
Author

t351206 commented Jul 27, 2020

@TheLastProject : Thanks for the test proposals!

ad 1) I think I need some help here.
ad 2) see importWithNoStarredField
ad 3) see multipleCardsExportImportSomeStarred and also implicitly included in other tests
ad 4) see importWithInvalidStarField
ad 5) see addFourLoyaltyCardsTwoStarred
ad 6) no starStatus will be exported for single cards, starStatus will be automatically set to 0 for import; covered with existing test
ad 7) I think this is an already existing issue. There is nothing implemented to avoid import of bad data (store, note, cardId, barcode can be null). I fixed it with an additional check if one of the four values is null. Furthermore I added two tests.

@t351206
Copy link
Author

t351206 commented Jul 27, 2020

How can I test clicking on the star?

shadowOf(activity).clickMenuItem(R.id.action_star_unstar) is producing an exception:
java.lang.NullPointerException at org.robolectric.fakes.RoboMenuItem.setIcon(RoboMenuItem.java:107) at protect.card_locker.LoyaltyCardViewActivity.setStarInDB(LoyaltyCardViewActivity.java:370) at protect.card_locker.LoyaltyCardViewActivity.onOptionsItemSelected(LoyaltyCardViewActivity.java:359) at android.app.Activity.onMenuItemSelected(Activity.java:2914) at androidx.fragment.app.FragmentActivity.onMenuItemSelected(FragmentActivity.java:384) at androidx.appcompat.app.AppCompatActivity.onMenuItemSelected(AppCompatActivity.java:219) at org.robolectric.shadows.ShadowActivity.clickMenuItem(ShadowActivity.java:502) at protect.card_locker.LoyaltyCardViewActivityTest.checkPushStarIcon(LoyaltyCardViewActivityTest.java:718) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:600) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.robolectric.internal.SandboxTestRunner$2.evaluate(SandboxTestRunner.java:260) at org.robolectric.internal.SandboxTestRunner.runChild(SandboxTestRunner.java:130) at org.robolectric.internal.SandboxTestRunner.runChild(SandboxTestRunner.java:42) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.robolectric.internal.SandboxTestRunner$1.evaluate(SandboxTestRunner.java:84) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58)

t351206 added 2 commits July 30, 2020 14:12
LoyaltyCardViewActivity adapted (onPrepareOptionsMenu used)

Signed-off-by: t351206 <[email protected]>
@t351206
Copy link
Author

t351206 commented Jul 30, 2020

@TheLastProject: I think I solved the testing issue! Test "checkPushStarIcon" is working as expected!

@t351206 t351206 marked this pull request as ready for review July 30, 2020 19:29
@TheLastProject
Copy link
Contributor

Sorry I didn't get back to you in your last reply, things have been a bit chaotic here and I forgot. Let me build your branch and play around with it a bit on my phone :)

Copy link
Contributor

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

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

Found two little issues that I think need changing.

Also, I think it would be really great if the home screen made it clear somehow what is a favourite. Like, showing the star symbol on the right of your favourite cards

app/src/main/res/values-de/strings.xml Outdated Show resolved Hide resolved
@TheLastProject
Copy link
Contributor

Have a few more comments but the code seems to work quite well and I do like it a lot :)

t351206 added 4 commits July 31, 2020 14:14
tests adapted
content description for star image added

Signed-off-by: t351206 <[email protected]>
Signed-off-by: t351206 <[email protected]>
Signed-off-by: t351206 <[email protected]>
Copy link
Contributor

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

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

Well, I am happy with it now. Let's hope @brarcher finds some time soon to take a look at this too :)

@TheLastProject
Copy link
Contributor

Hi @t351206,

After talking with @brarcher I ended up forking Loyalty Card Locker. I would love to include your feature in my fork (which I will soon publish after I have a new icon). Could you please make this pull request on https://github.com/TheLastProject/Catima?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants