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

Activity filter memo and maximum amount #2807

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

Conversation

Mayank2808sharma
Copy link

Description

Relates to issue: ZEUS-83

I added the functionality to filter the activity list using memo/note and maximum amount.

Screenshot 2025-02-06 at 10 43 45 PM

This pull request is categorized as a:

  • [✅] New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • [✅ ] I’ve run yarn run tsc and made sure my code compiles correctly
  • [✅] I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
  • [✅] I’ve run yarn run prettier and made sure my code is formatted correctly
  • [✅] I’ve run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • [✅] Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • [✅] Android (version: Android 15)
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

  • Embedded LND
  • LND (REST)
  • LND (Lightning Node Connect)
  • Core Lightning (CLNRest)
  • LndHub
  • [DEPRECATED] Core Lightning (c-lightning-REST)
  • [DEPRECATED] Core Lightning (Spark)
  • [DEPRECATED] Eclair

Locales

  • I’ve added new locale text that requires translations
  • [✅] I’m aware that new translations should be made on the ZEUS Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

endDate: undefined
endDate: undefined,
memo: '',
maximumAmount: Infinity
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to just have Infinity/No limit/No maximum be a placeholder for the text input

Copy link
Author

Choose a reason for hiding this comment

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

I will change it such that the maximumAmount textbox will show no limits as a placeholder.
Gonna Remove the Infinity one

@@ -33,6 +33,8 @@ export interface Filter {
minimumAmount: number;
startDate?: Date;
endDate?: Date;
memo: string;
maximumAmount: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would probably just slot maximumAmount right below minimumAmount


expect(filteredActivities.length).toBe(1);
expect(filteredActivities[0].getNote).toBe('Payment for invoice');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Love to see the test cases here!

@kaloudis
Copy link
Contributor

kaloudis commented Feb 6, 2025

nit: the merge commit is OK, but would be better to you rebase the PR instead

@kaloudis kaloudis added the Activity Activity view label Feb 6, 2025
@kaloudis kaloudis added this to the v0.10.0 milestone Feb 6, 2025
@kaloudis
Copy link
Contributor

kaloudis commented Feb 6, 2025

nit: you can just put an x inbetween the [ ] brackets in PR description to check the boxes

@myxmaster
Copy link
Contributor

Didn't test yet, but this looks pretty nice!

Can you please localize the placeholders as well?

@myxmaster
Copy link
Contributor

The Minimum Amount (sats) placeholder is gone:

grafik

@myxmaster
Copy link
Contributor

The Memo/Note filter only works for notes, not for memos.

@Mayank2808sharma
Copy link
Author

Mayank2808sharma commented Feb 8, 2025

The Minimum Amount (sats) placeholder is gone:

grafik

@myxmaster The Minimum Amount (sats) is 0 right? or should i make a placeholder similar to Maximum Amount sats for Minimum amount

@Mayank2808sharma
Copy link
Author

The Memo/Note filter only works for notes, not for memos.

Where do I get all the data for memos and notes for testing purposes? As for now, I used lightning receive to create invoices to fill the activity list.
@myxmaster

@myxmaster
Copy link
Contributor

The Minimum Amount (sats) is 0 right? or should i make a placeholder similar to Maximum Amount sats for Minimum amount

Currently (before this PR) we have "0" as placeholder in this input and we should keep it that way imo.

@myxmaster
Copy link
Contributor

Where do I get all the data for memos and notes for testing purposes? As for now, I used lightning receive to create invoices to fill the activity list.

Yeah, you can just make invoices with memo... Notes are something else. (But I definitely like the idea to combine memo and notes for the filter.)

@Mayank2808sharma
Copy link
Author

The Minimum Amount (sats) is 0 right? or should i make a placeholder similar to Maximum Amount sats for Minimum amount

Currently (before this PR) we have "0" as placeholder in this input and we should keep it that way imo.

alright so for minimum I will make it 0 and for maximum no limit

locales/en.json Outdated Show resolved Hide resolved
@myxmaster
Copy link
Contributor

Looking good!

Just one more thought:
I'm not sure if we should trim the memo filter input. It could be reasonable for users to filter two words, right?

@@ -128,6 +136,24 @@ class ActivityFilterUtils {
(activity) => activity.getDate.getTime() < endDate.getTime()
);
}
if ((filter.memo || '').trim() !== '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

|| '' should be redundant since memo is initialized with '' ?

Copy link
Author

Choose a reason for hiding this comment

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

Initally I used filter.memo.trim() !== '' and a few test cases were failing so to tackle that I used (filter.memo || '').trim() !== ''

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you need to add memo: '' to getDefaultFilter() in utils\ActivityFilterUtils.test.ts.

@Mayank2808sharma
Copy link
Author

Looking good!

Just one more thought: I'm not sure if we should trim the memo filter input. It could be reasonable for users to filter two words, right?

Trimming helps prevent accidental spaces but still allows multi-word filtering like 'payment memo'. Let me know if you'd still prefer to remove it

@myxmaster
Copy link
Contributor

myxmaster commented Feb 10, 2025

Trimming helps prevent accidental spaces but still allows multi-word filtering like 'payment memo'.

Right now you can only paste two words, you cannot type it, because it will remove the space immediately.

Why do we even care if people filter (only space char)? If it was on accident and they don't understand that there is a space char in that filter input, they can always start fresh by using the delete icon. I'm not sure this user error prevention actually solves any problem.

@myxmaster
Copy link
Contributor

LGTM now :)

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

I have noticed the Activity view freezes after we set up the Memo/Note field.

@shubhamkmr04
Copy link
Contributor

frequently getting the warning
Simulator Screenshot - iPhone 16 Pro - 2025-02-11 at 15 32 52

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

Not able to type in anything in the memo/note field , recreated it in both emulator and physical android device

Screen.Recording.2025-02-11.at.5.05.39.PM.mov

@myxmaster
Copy link
Contributor

myxmaster commented Feb 11, 2025

Yes, I see the TypeError: Cannot read property 'toLowerCase' of undefined as well now. (I probably didn't see it before because filter.memo was already set during my testing.)

It happens because we are not merging the filters, which we get from Storage, with DEFAULT_FILTERS in getFilters(). That is needed when we open Activity view with a new Zeus version (where we implemented a new filter) for the first time (because at this point the new filter cannot exist in Storage). I think we should update the method to this:

    @action
    public async getFilters() {
        try {
            const filters = await Storage.getItem(ACTIVITY_FILTERS_KEY);
            if (filters) {
                const parsedFilters = JSON.parse(filters, (key, value) =>
                    (key === 'startDate' || key === 'endDate') && value
                        ? new Date(value)
                        : value
                );
                this.filters = { ...DEFAULT_FILTERS, ...parsedFilters };
            } else {
                console.log('No activity filters stored');
                this.filters = DEFAULT_FILTERS;
            }
        } catch (error) {
            console.log('Loading activity filters failed', error);
        }

        return this.filters;
    }

 
@shubhamkmr04:

Not able to type in anything in the memo/note field

This is something else and has nothing to do with this PR. I think it is some old workaround for the virtual keyboard covering some stuff.
Please test this:
Start current Zeus release (not on this feature branch here) on Android emulator or physical device, open Filter view, do NOT scroll down all the way, click on Minimum Amount (sats) input.
-> Keyboard will appear and immediately disappear and the view will scroll down.
-> Now, when it is scrolled down all the way, you can click on the input again and the keyboard will appear and stay.

Edit:
But ok, I notice that for you the keyboard doesn't even stay when you ARE scrolled down all the way... We should investigate further...

@Mayank2808sharma
Copy link
Author

Hello @shubhamkmr04 and @myxmaster

I believe the issue TypeError: Cannot read property 'toLowerCase' of undefined can be resolved by using filter.memo?.toLowerCase() || '';

Regarding the other two issues, I’m not entirely sure why they’re occurring.

But if we put the memo filter textbox above the min and max, the keyboard opens up

Screenshot 2025-02-11 at 10 57 31 PM Screenshot 2025-02-11 at 10 57 44 PM

Please let me know the next steps or should I put the memo box above

@Mayank2808sharma
Copy link
Author

Screenrecording_20250211_231918.mp4

@myxmaster
Copy link
Contributor

I believe the issue TypeError: Cannot read property 'toLowerCase' of undefined can be resolved by using filter.memo?.toLowerCase() || '';

Yes, it should fix it, but it masks the underlying issue of incomplete filter data. Imo the updated getFilters() method is the cleaner solution.

@myxmaster
Copy link
Contributor

Regarding the keyboard appear/disappear issue:
Instead of moving the input up, I think we should replace <FlatList> with <ScrollView>. That fixes the issue on Android at least, no idea about iOS.

Only minor code changes needed:

                <ScrollView>
                    {FILTERS.map((item, index) => {
                        if (!item.condition) return null;

                        return (
                            <React.Fragment key={index}>
                                <ListItem
                                    containerStyle={{
                                        borderBottomWidth: 0,
                                        backgroundColor: 'transparent'
                                    }}
                                >
                                    [...]
                                </ListItem>
                                {index < FILTERS.length - 1 &&
                                    this.renderSeparator()}
                            </React.Fragment>
                        );
                    })}
                </ScrollView>

@Mayank2808sharma
Copy link
Author

@myxmaster

so should I make changes according to your suggestion?

@myxmaster
Copy link
Contributor

Merging the filters with DEFAULT_FILTERS in getFilters()
-> for sure

Replacing FlatList with ScrollView
-> Well, I don't see any disadvantages in using ScrollView instead of FlatList, and the Keyboard appearance issues are completely gone for me when using ScrollView. Thoughts @shubhamkmr04 @kaloudis ?

@kaloudis
Copy link
Contributor

Merging the filters with DEFAULT_FILTERS in getFilters() -> for sure

Replacing FlatList with ScrollView -> Well, I don't see any disadvantages in using ScrollView instead of FlatList, and the Keyboard appearance issues are completely gone for me when using ScrollView. Thoughts @shubhamkmr04 @kaloudis ?

We should try it out. I think it will work without issue.

@myxmaster
Copy link
Contributor

Tested -> no issues now when coming from Zeus version without the new filter.

ScrollView also working perfectly fine, no keyboard issues for me (tested with Android emulator only).

@shubhamkmr04
Copy link
Contributor

@Mayank2808sharma so for iOS we still this small issue when we can't see the fields that are in the bottom of the view because keyboard won't go away

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-14.at.20.57.07.mp4

It should be a small fix, you can take reference from here

@Mayank2808sharma
Copy link
Author

@Mayank2808sharma so for iOS we still this small issue when we can't see the fields that are in the bottom of the view because keyboard won't go away

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-14.at.20.57.07.mp4
It should be a small fix, you can take reference from here

Thanks for the resource. I have made the changes, let me know what you think

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

Successfully merging this pull request may close these issues.

4 participants