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 rented by me and include rentable nodes filter #3852

Merged
merged 13 commits into from
Feb 23, 2025

Conversation

samaradel
Copy link
Contributor

@samaradel samaradel commented Jan 30, 2025

Description

Add rented by me and include rentable nodes filter

Changes

Screencast.from.17-02-25.20.42.05.webm

Updated filters

image

Related Issues

Tested Scenarios

  • Switch Rented by me (will show only the rented nodes)
  • Switch Include rentable nodes (will show only the rentable nodes)
  • Switch both (will show the rentable nodes and rented nodes )

Documentation PR

To consider

Preliminary Checks:

  • Preliminary Checks
    • Does it completely address the issue linked?
    • What about edge cases?
    • Does it meet the specified acceptance criteria?
    • Are there any unintended side effects?
    • Does the PR adhere to the team's coding conventions, style guides, and best practices?
    • Does it integrate well with existing features?
    • Does it impact the overall performance of the application?
    • Are there any bottlenecks or slowdowns?
    • Has it been optimized for efficiency?
    • Has it been adequately tested with unit, integration, and end-to-end tests?
    • Are there any known defects or issues?
    • Is the code well-documented?
    • Are changes to documentation reflected in the code?

UI Checks:

  • UI Checks
    • If a UI design is provided/ does it follow it?
    • Does every button work?
    • Is the data displayed logical? Is it what you expected?
    • Does every validation work?
    • Does this interface feel intuitive?
    • What about slow network? Offline?
    • What if the gridproxy/graphql/chain is failing?
    • What would a first time user have a hard time navigating here?

Code Quality Checks:

  • Code Quality Checks
    • Code formatted/linted? Are there unused imports? .. etc
    • Is there redundant/repeated code?
    • Are there conditionals that are always true or always false?
    • Can we write this more concisely?
    • Can we reuse this code? If yes, where?
    • Will the changes be easy to maintain and update in the future?
    • Can this code become too complex to understand for other devs?
    • Can this code cause future integration problems?

Testing Checklist

  • Does it Meet the specified acceptance criteria.
  • Test if changes can affect any other functionality.
  • Tested with unit, integration, and end-to-end tests.
  • Tested the un-happy path and rollback scenarios.
  • Documentation updated to meet the latest changes.
  • Check automated tests working successfully with the changes.
  • Can be covered by automated tests.

General Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring
  • Screenshots/Video attached (needed for UI changes)

Sorry, something went wrong.

Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

I don't think we need to have a rentedbyMe to be passed to the filters; we can have computed value that returns the twinid or undifinded based on the rentedByMe toggle value and pass it to the rentedby

@samaradel samaradel requested a review from 0oM4R February 19, 2025 17:45
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

freeflow is not updated with new changes, please check

@samaradel samaradel requested a review from 0oM4R February 20, 2025 14:55
@samaradel
Copy link
Contributor Author

It's not a part of the dashboard applications anymore

@0oM4R
Copy link
Contributor

0oM4R commented Feb 20, 2025

It's not a part of the dashboard applications anymore

It should be added; we may reuse the application again; then the component should be updated.

Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

will test caprover worker later when we have node that have ipv4 rentable

Co-authored-by: Omar Kassem <omarksm09@gmail.com>
@samaradel samaradel marked this pull request as draft February 20, 2025 17:11
@samaradel samaradel requested a review from 0oM4R February 20, 2025 17:43
@samaradel samaradel marked this pull request as ready for review February 20, 2025 17:44
@0oM4R
Copy link
Contributor

0oM4R commented Feb 23, 2025

I suggest changing the include rentable nodes title to be Renatble
what if the use only toggle on the include rentable nodes so what are we including it on? it will list only rentable nodes
also Rented by me should be on by default, as @xmonader ask

should we update the tooltip to be learn more about rentable nodes?

image

make sure to pull the development branch

@samaradel
Copy link
Contributor Author

I agree with renaming but for turning on my rented nodes filter will list only my nodes so if it doesn't satisfy the requirements it will show no nodes found.
The current behavior "without turning on any filter" will show the rented nodes on the top of nodes, I think it's the behavior you want.

@samaradel samaradel marked this pull request as ready for review February 23, 2025 11:30
@samaradel samaradel marked this pull request as draft February 23, 2025 11:49
@samaradel samaradel marked this pull request as ready for review February 23, 2025 12:05
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

I dont think we need nodes word here as we do in certified

image

Rented by me (only)
Rentable
Certified

also the tooltip i think should changed as i mentioned
image

@ehab-hassan what do you think?

@xmonader xmonader merged commit 96da187 into development Feb 23, 2025
10 checks passed
@xmonader xmonader deleted the development_nodeselector branch February 23, 2025 12:53
@0oM4R 0oM4R mentioned this pull request Feb 23, 2025
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.

None yet

3 participants