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.

Sign up for GitHub

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

Jump to bottom

Check schedule flags validity also when creating a schedule from scratch #865

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Check schedule flags validity also when creating a schedule from scratch #865

merged 3 commits into from
Nov 6, 2023

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Nov 6, 2023

Rationale

Fix #864

Changes

  • schedule flags are also checked when creating a recipe
  • freecodecamp test case has been adapted to:
    • fix "normal" test case which was indeed not ok since mandatory flags were not provided at schedule creation
    • create an abnormal test case to exercise new check / avoid future regressions (this has been done on freecodecamp for simplicity, any scraper could be used indeed)
  • an abnormal test case has also been created for the more generic schedule creation tests
  • generic schedule creation tests have been fixed to ensure they are failing for the expected reason

@benoit74 benoit74 force-pushed the check_flags_at_schedule_creation branch from 290c589 to 6faccfc Compare November 6, 2023 08:12
@benoit74 benoit74 marked this pull request as ready for review November 6, 2023 08:19
@benoit74 benoit74 requested a review from rgaudin November 6, 2023 08:19
@benoit74
Copy link
Collaborator Author

benoit74 commented Nov 6, 2023 •
edited
Loading

Indeed there is maybe a thing to enhance in this PR, we validate schemas twice now (once for global schema and once for offliner schema).

This has the disadvantage that if we have two errors, one in the global schema and one in the offliner schema, the error message will only contain the error in the global schema at first. And once this is fixed, it will display the second error in the offliner schema.

Maybe we should trigger both checks in all situations and merge the two lists of error messages before returning the errors. Or maybe the price to pay is too high for a situation which will most probably never occur in reality, at least with current UIs.

rgaudin
rgaudin requested changes Nov 6, 2023
View reviewed changes
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

LGTM but you left a few debug prints

dispatcher/backend/src/tests/integration/routes/schedules/test_freecodecamp.py Outdated Show resolved Hide resolved
dispatcher/backend/src/tests/integration/routes/schedules/test_freecodecamp.py Outdated Show resolved Hide resolved
dispatcher/backend/src/tests/integration/routes/schedules/test_schedule.py Outdated Show resolved Hide resolved
dispatcher/backend/src/tests/integration/routes/schedules/test_schedule.py Outdated Show resolved Hide resolved
dispatcher/backend/src/tests/integration/routes/schedules/test_schedule.py Outdated Show resolved Hide resolved
@benoit74
Copy link
Collaborator Author

benoit74 commented Nov 6, 2023

I left those debug print intentionally (to help should the test fail at some point), but I don't mind to remove them, I have no real PoV of whether this is a good idea or not. I find it useful to avoid to have to debug the test locally to understand what went wrong ... but in fact this is usually needed anyway. I will remove them for now.

@rgaudin
Copy link
Member

rgaudin commented Nov 6, 2023

I left those debug print intentionally (to help should the test fail at some point), but I don't mind to remove them, I have no real PoV of whether this is a good idea or not. I find it useful to avoid to have to debug the test locally to understand what went wrong ... but in fact this is usually needed anyway. I will remove them for now.

If those are useful, we can left them in. We shall document somewhere (bootstrap) that we allow this and add corresponding relaxing rule for tests (in other projects we lint the tests)

@benoit74
Copy link
Collaborator Author

benoit74 commented Nov 6, 2023

I'm not sure they are really useful. My point was only to stress this might be intentional ^^

@benoit74 benoit74 requested a review from rgaudin November 6, 2023 10:56
rgaudin
rgaudin approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

👍

@rgaudin rgaudin merged commit 7ce4f31 into main Nov 6, 2023
5 checks passed
@rgaudin rgaudin deleted the check_flags_at_schedule_creation branch November 6, 2023 11:02
@benoit74 benoit74 mentioned this pull request Nov 6, 2023
Closed
benoit74 added a commit that referenced this pull request Nov 6, 2023
@benoit74
Revert "Merge pull request #865 from openzim/check_flags_at_schedule_…
5883e60 
…creation"

Change has too much impact on youzim.it (and WP1) for now, we have to
get prepared before releasing this.

This reverts commit 7ce4f31, reversing
changes made to aa387e3.
benoit74 added a commit that referenced this pull request Nov 9, 2023
@benoit74
Reapply "Merge pull request #865 from openzim/check_flags_at_schedule…
2a86d30 
…_creation"

This reapply commit 7ce4f31
@benoit74 benoit74 self-assigned this Nov 9, 2023
@benoit74 benoit74 mentioned this pull request Nov 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgaudin rgaudin rgaudin approved these changes

Assignees

@benoit74 benoit74

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schedule configuration flags must also be validated at schedule creation
2 participants
@benoit74 @rgaudin