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

fix: #842 - "undefined" value on dashboard assets graph #844

Closed

Conversation

AurelienConte
Copy link

@AurelienConte AurelienConte commented Jun 7, 2024

Fixing issue of FractionalPart displaying "undefined" in the pie chart element used to display assets and debts on the dashboard

EDIT : Please, do not hesitate to tell me about the testing part, i'm not familiar with ruby and tools in this ecosystem. I'm still learning and maybe i used them in a not proper way !

@AurelienConte AurelienConte marked this pull request as ready for review June 7, 2024 20:37
@zachgoll
Copy link
Collaborator

@AurelienConte do you plan to continue work on this? No worries either way, just let me know!

@AurelienConte
Copy link
Author

Sorry i was on vacation, I just come back today at work !
I plan to come back on the pull request tomorrow or Wednesday 😄

@zachgoll
Copy link
Collaborator

@AurelienConte sounds great!

…return also how the symbol should be displayed
app/helpers/value_groups_helper.rb Outdated Show resolved Hide resolved
app/helpers/value_groups_helper.rb Outdated Show resolved Hide resolved
@AurelienConte
Copy link
Author

I made changes to the feat, if you think this is good to you i will start modifying my tests to match the code changes

@AurelienConte
Copy link
Author

I made changes to the feat, if you think this is good to you i will start modifying my tests to match the code changes

@zachgoll did you find any time to check my work ?

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

The code is looking good here. We will just need to move the test and fix the lint errors (you can use bin/erblint to check this locally) and then we can get it merged!

entire_text = value_element.find(:xpath, '..').text # Get full text of the <p> element
extracted_value = entire_text.strip.gsub(/[^\d.]/, '').sub(/^0+/, '') # Removing non-digit characters except the decimal point
assert_equal expected_value, extracted_value, "The pie chart value is not displayed correctly"
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should test this functionality in a new system test called test/system/dashboard_test.rb, and for now, we can have a single test:

class DashboardTest < ApplicationSystemTestCase
  test "displays pie chart" do 
    # click on assets
    # assert formatted value
    # click on debts 
    # assert formatted value
  end
end

@AurelienConte
Copy link
Author

Perfect, I'll start testing when I have some time

@AurelienConte
Copy link
Author

@zachgoll sorry but i think i can not find enough time to finish this ... can you finish this for me ?

@zachgoll
Copy link
Collaborator

@AurelienConte yep, I can finish it up! Thanks for letting me know.

@zachgoll
Copy link
Collaborator

Going to close this out for now and will circle back when I finish some of the investment work I'm doing.

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