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

Feat:Added field to show automatic scheduling status #873

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

gpuligundla
Copy link

@gpuligundla gpuligundla commented Nov 20, 2023

Rationale

Fix #816

Changes

  • Added enabled field to schedulelight schema & retrieved its value from Postgres.
  • Added a pause icon to show it in the front end.

Screenshot:

Screenshot 2023-11-20 122716

@benoit74 benoit74 self-requested a review November 21, 2023 16:47
@benoit74
Copy link
Collaborator

@gpuligundla do you confirm this is ready for review?

@benoit74
Copy link
Collaborator

NOTA: I updated your first comment to use the "Fix #xxx" syntax, so that the issue will automatically be closed when the PR is merged

@gpuligundla
Copy link
Author

@gpuligundla do you confirm this is ready for review?

yes

Copy link
Collaborator

@benoit74 benoit74 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 please do not merge main into your branch to retrieve changes, instead rebase your branch on top of latest main changes (typically with git rebase main when on your branch). This is a highly debatable choice of course, but it is our convention ^^

Thank you for this very good first PR anyway.

@gpuligundla gpuligundla force-pushed the feat/automatic-scheduling-status branch 2 times, most recently from b440f31 to 0590f63 Compare November 21, 2023 20:23
@gpuligundla
Copy link
Author

LGTM, but please do not merge main into your branch to retrieve changes, instead rebase your branch on top of latest main changes (typically with git rebase main when on your branch). This is a highly debatable choice of course, but it is our convention ^^

Thank you for this very good first PR anyway.

Done

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Thank you! I'm just asking @rgaudin for a final pass since he has more expertise than me on this kind of UI stuff and previous discussions that occurred on the Zimfarm.

@benoit74
Copy link
Collaborator

Just to clarify a bit the PR workflow (detailed in https://github.com/openzim/overview/wiki/openZIM-workflow), especially regarding changes on main branch while you are developing for all openzim/kiwix/offspot repos:

  • do not forgot to ask for a review once your PR is ready (otherwise it is difficult for us to know if it is really ready or still a WIP); you might add us as reviewers directly (for zimfarm, either rgaudin or myself is OK) or simply add a comment asking for help (when working on other repos where
  • once the PR has been submitted a first time to review, do not mind any changes that will happen on main branch anymore (they will be rare anyway) ; in general, do not rebase or merge, since it will make the review work much more tedious ; before submitting your first review you might do the rebase (especially if it took you weeks to finalize the PR)
  • fix all requested changes on your branch as-is (based on a potential old commit of main branch) ; ask again for review once fixes have been done or answers provided (not all our comments require a change, sometimes we might be wrong as well ^^)
  • once PR is ready, the reviewer (usually, especially when this is obvious and also because he usually has more knowledge of what happened on main) or the author (sometimes, when lots of conflict have to be dealt with) will take care of rebasing the PR branch on latest commit of main branch

But again, don't be afraid of all this, this is just small conventions to facilitate everyone asynchronous work, avoid misunderstandings and your help is still valuable and welcomed!

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 ; thank you for this great first contribution 👍

@gpuligundla
Copy link
Author

Just to clarify a bit the PR workflow (detailed in https://github.com/openzim/overview/wiki/openZIM-workflow), especially regarding changes on main branch while you are developing for all openzim/kiwix/offspot repos:

  • do not forgot to ask for a review once your PR is ready (otherwise it is difficult for us to know if it is really ready or still a WIP); you might add us as reviewers directly (for zimfarm, either rgaudin or myself is OK) or simply add a comment asking for help (when working on other repos where
  • once the PR has been submitted a first time to review, do not mind any changes that will happen on main branch anymore (they will be rare anyway) ; in general, do not rebase or merge, since it will make the review work much more tedious ; before submitting your first review you might do the rebase (especially if it took you weeks to finalize the PR)
  • fix all requested changes on your branch as-is (based on a potential old commit of main branch) ; ask again for review once fixes have been done or answers provided (not all our comments require a change, sometimes we might be wrong as well ^^)
  • once PR is ready, the reviewer (usually, especially when this is obvious and also because he usually has more knowledge of what happened on main) or the author (sometimes, when lots of conflict have to be dealt with) will take care of rebasing the PR branch on latest commit of main branch

But again, don't be afraid of all this, this is just small conventions to facilitate everyone asynchronous work, avoid misunderstandings and your help is still valuable and welcomed!

sure, I'll follow it from next time. Thanks <3

@gpuligundla gpuligundla force-pushed the feat/automatic-scheduling-status branch from 0590f63 to 47d7918 Compare November 22, 2023 15:56
@benoit74 benoit74 merged commit 355f828 into openzim:main Nov 23, 2023
4 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.

Show recipe enable status in recipe list
3 participants