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

Add provider filter to superadmin #341

Merged
merged 12 commits into from
Mar 7, 2024

Conversation

AbdulWahab3181
Copy link
Contributor

@AbdulWahab3181 AbdulWahab3181 commented Mar 1, 2024

Describe your changes

https://www.loom.com/share/a5f1eaa0dfec44ed96f828cc0dd5852b

Issue ticket number and link

Closes #294

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested on Chrome and Firefox
  • I have tested on a mobile device
  • I have provided a screenshot or recording of changes in my PR if there were updates to the frontend

@AbdulWahab3181 AbdulWahab3181 marked this pull request as ready for review March 1, 2024 04:06
@ecurrencyhodler
Copy link
Contributor

Thanks for the video and also demonstrating scrolling.

I noticed that once a provider has been selected the other providers are hidden until clear is pressed.

Let's instead keep all providers visible. Just preserve the check marks if they've been selected.

@AbdulWahab3181
Copy link
Contributor Author

Thanks for the video and also demonstrating scrolling.

I noticed that once a provider has been selected the other providers are hidden until clear is pressed.

Let's instead keep all providers visible. Just preserve the check marks if they've been selected.

Done

https://www.loom.com/share/c13f3459d1914f0cb3a22d7e68cfae62

@ecurrencyhodler
Copy link
Contributor

LGTM Thanks. Let's get a code review.

</ProviderContianer>
))
) : (
<p>No provider with such alias</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

No provider available, should be message when the provider length is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elraphty Addressed

>
<ProviderContainer>
<ProvidersListContainer>
{providers && providers.length > 0 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add an intersection observer to load more providers on scroll when the provider length is more than 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @elraphty! Why is there a need for the Intersection Observer to load more providers on scroll when we are already receiving all provider data from the metrics/bounties endpoint call? We are not making another call for additional providers unless triggered by changes in the date filter, for example. So, my point is, since we already have all providers, it would be more appropriate to implement the Intersection Observer if we need to call the API to get more providers. Could you please correct me if I am wrong or guide me?

@elraphty
Copy link
Collaborator

elraphty commented Mar 4, 2024

Provider dropdown is not inline with the other dropdownsScreenshot

@AbdulWahab3181
Copy link
Contributor Author

@elraphty Addressed

image

@kevkevinpal
Copy link
Collaborator

kevkevinpal commented Mar 5, 2024

@elraphty @ecurrencyhodler I think we might want to do the filtering by provider from the backend doing it from the frontend will lead to inaccurate results, I would rather us do it by backend first instead of creating a frontend change that we'll need to fix in the future. I think we may have not scoped this issue well enough

let me know what you think

@AbdulWahab3181
Copy link
Contributor Author

@kevkevinpal We are already doing filtering by provider from the backend as you can see here

@elraphty
Copy link
Collaborator

elraphty commented Mar 5, 2024

@kevkevinpal We are already doing filtering by provider from the backend as you can see here

Take a look at @AbdulWahab3181 https://www.loom.com/share/818e0e9267a84332baabc9949edd847f the provider list is inaccurate, I will go with @kevkevinpal suggestion the provider list should come from the backend.

@AbdulWahab3181
Copy link
Contributor Author

AbdulWahab3181 commented Mar 5, 2024

@elraphty, I understand why you're only seeing two providers. We're currently displaying unique providers sourced from the metrics/bounties endpoint. When you select a 90-day or custom date range, the metrics/bounties endpoint is called with a limit of 20. Consequently, we receive 20 bounties related to 2 providers. If you wish to view other providers, please navigate to the next pages, as demonstrated in the attached video.

You're correct that the provider list comes from the backend for greater accuracy, but this wasn't within the scope of our current task. Additionally, there isn't an endpoint available in the backend that provides the list of providers. I propose closing this task after merging and creating two new tasks: one for the backend to provide the list of providers and another for the frontend to display these providers.

https://www.loom.com/share/65cbad64dac84ebdbdaf9e0ebaee0c3b

@elraphty elraphty force-pushed the superadmin-provider-filter branch from 4d0e0b9 to 62f9616 Compare March 5, 2024 17:43
@kevkevinpal
Copy link
Collaborator

@AbdulWahab3181 so we can bump your bounty amount up 200k sats and can you then implement a filter on the API instead to filter by providers? Right now we are filtering by the bounties we have on the frontend which will lead to poor results

@elraphty can work on the backend to create the new query param for the provider filter

@kevkevinpal
Copy link
Collaborator

For now we can keep this PR up

@AbdulWahab3181
Copy link
Contributor Author

@AbdulWahab3181 so we can bump your bounty amount up 200k sats and can you then implement a filter on the API instead to filter by providers? Right now we are filtering by the bounties we have on the frontend which will lead to poor results

@elraphty can work on the backend to create the new query param for the provider filter

@kevkevinpal Thanks for considering the increase in bounty. Sure, I would do that once the provider list endpoint is ready on the backend.

@ecurrencyhodler
Copy link
Contributor

Updated bounty. I'll create a ticket for raph: https://community.sphinx.chat/bounty/1582

@elraphty
Copy link
Collaborator

elraphty commented Mar 6, 2024

@AbdulWahab3181 ${Host}/metrics/bounties/providers?page=1&Open=false&Assigned=true&Paid=true&direction=desc&limit=5, this will return the providers list

@AbdulWahab3181
Copy link
Contributor Author

@elraphty @ecurrencyhodler I am receiving an 'access denied' message when trying to access the admin page. Could you please investigate on the backend?

image image

@AbdulWahab3181
Copy link
Contributor Author

I'm also encountering a 404 error when calling the metrics/bounties/providers endpoint. It was functioning properly just a minute before the access denied issues occurred

image

@elraphty
Copy link
Collaborator

elraphty commented Mar 7, 2024

@AbdulWahab3181 This worked for me https://people-test.sphinx.chat/metrics/bounties/providers?page=1&Open=false&Assigned=true&Paid=true&direction=desc&limit=5

Just make sure you pass the x-jwt header and the request body should be

{
    "start_date": "1709209136", 
    "end_date": "1709813936"
}

Screenshot

@AbdulWahab3181
Copy link
Contributor Author

@elraphty Now, It's working for me as well

@AbdulWahab3181
Copy link
Contributor Author

@elraphty I have Implemented provider list endpoint. Could you please have a look, also review and merge the PR?

https://www.loom.com/share/f1367dabe7654d0bad2759777f373829

route_hint?: string;
owner_route_hint?: string;
owner_contact_key?: string;
price_to_meet: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@AbdulWahab3181 Just import the Person interface from main.ts, instead of duplicating it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AbdulWahab3181 Just import the Person interface from main.ts, instead of duplicating it.

Nice work @AbdulWahab3181 just fix this, so I can merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elraphty Thanks, Addressed

@elraphty elraphty merged commit 8e1501d into stakwork:master Mar 7, 2024
5 checks passed
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.

Add provider filter to superadmin
4 participants