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

Separate exercise list into current/previous exercises #69

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

senier
Copy link
Contributor

@senier senier commented Dec 19, 2024

I chose the previous 31 days as the cutoff for current exercises, but have no strong opinion here...

@treiher
Copy link
Owner

treiher commented Dec 19, 2024

That's a good idea. The 31 days cutoff seems fine to me. An alternative would be to consider as current only those exercises that are part of an active routine, or show all exercises that fulfill the former or latter condition. I have also no strong opinion here.

I think it would be nicer if we omitted the heading "Current exercises". A similar approach is used for routines: There is a heading "Archive", but no heading "Current routines".

We could also add a tooltip to explain what "Previous exercises" means (something like "Exercises that have not been done in the last 31 days"). The common::view_element_with_description function could be used for that.

@senier senier force-pushed the topic/senier/current_exercises branch from fe5a6fe to 59b5bec Compare December 19, 2024 21:58
@senier
Copy link
Contributor Author

senier commented Dec 19, 2024

An alternative would be to consider as current only those exercises that are part of an active routine, or show all exercises that fulfill the former or latter condition. I have also no strong opinion here.

I'm in favor of using the 31 days cutoff for now and see how it goes. This is easily changed if we feel the behavior could be better.

I think it would be nicer if we omitted the heading "Current exercises". A similar approach is used for routines: There is a heading "Archive", but no heading "Current routines".

We could also add a tooltip to explain what "Previous exercises" means (something like "Exercises that have not been done in the last 31 days"). The common::view_element_with_description function could be used for that.

Good suggestions - I changed the implementation accordingly.

@senier senier force-pushed the topic/senier/current_exercises branch from 59b5bec to 4827fbd Compare December 21, 2024 12:21
@senier
Copy link
Contributor Author

senier commented Dec 21, 2024

What would be the best way to fix the e2e tests?

FAILED tests/e2e/web_test.py::test_exercises_add - selenium.common.exceptions.TimeoutException: Message:
FAILED tests/e2e/web_test.py::test_exercises_edit - selenium.common.exceptions.TimeoutException: Message:
FAILED tests/e2e/web_test.py::test_exercises_delete - selenium.common.exceptions.TimeoutException: Message:

As the test data is static (and in the past), all exercises likely show up in "Previous exercises" and don't seem to be found by page.wait_for_table_value() (does this only match the first table in the page?). Making the test data in tests/data.py relative to the current date fixes that issue, but breaks the tests/backend/api_test.py which compares against the same static values. Another approach could be to fake the current date when running the backend for testing...

@treiher
Copy link
Owner

treiher commented Dec 23, 2024

As the test data is static (and in the past), all exercises likely show up in "Previous exercises" and don't seem to be found by page.wait_for_table_value() (does this only match the first table in the page?).

The issue is that page.wait_for_table_value() only matches the first table. Also unused exercises will be listed as current exercises. As we have this case in the test data, there are now two tables. I have improved page.wait_for_table_value() in d8acc9c, so that the table can be specified. At least test_exercises_add can be fixed by changing the table number. I guess it's the same for the other ones.

Making the test data in tests/data.py relative to the current date fixes that issue, but breaks the tests/backend/api_test.py which compares against the same static values. Another approach could be to fake the current date when running the backend for testing...

We could make the test data parametrizable, so that a start date can be defined. A fixed value could be used for the API tests and a dynamic one for the E2E tests. But I think it would be also fine to not explicitly test this feature in the E2E tests.

Copy link
Owner

Choose a reason for hiding this comment

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

Please add a changelog entry for this change.

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