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

Implement bulk scheduling (#32j3gu0) #314

Merged
merged 46 commits into from
Jan 18, 2023
Merged

Implement bulk scheduling (#32j3gu0) #314

merged 46 commits into from
Jan 18, 2023

Conversation

alsoK2maan
Copy link
Contributor

@alsoK2maan alsoK2maan commented Dec 16, 2022

Related Issues

Closes #304

Short Description and Why It's Useful

The bulk job scheduler enables scheduling multiple jobs at once. The user can select for which store and for which configs they want to schedule the jobs. It aids in the process of setting up the instance as one can ease the process by scheduling multiple jobs at once instead of manually scheduling like before.

Screenshots of Visual Changes before/after (If There Are Any)

Job Manager - HotWax Commerce
Job Manager - HotWax Commerce (1)
Job Manager - HotWax Commerce (2)

IMPORTANT NOTICE - Remember to add changelog entry

Contribution and Currently Important Rules Acceptance

@alsoK2maan alsoK2maan marked this pull request as ready for review December 21, 2022 11:52
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/locales/en.json Outdated Show resolved Hide resolved
src/store/modules/job/actions.ts Outdated Show resolved Hide resolved
src/store/modules/job/actions.ts Outdated Show resolved Hide resolved
src/views/BulkEditor.vue Outdated Show resolved Hide resolved
src/views/BulkEditor.vue Outdated Show resolved Hide resolved
src/views/BulkEditor.vue Show resolved Hide resolved
src/views/SelectJobsModal.vue Show resolved Hide resolved
src/views/SelectJobsModal.vue Outdated Show resolved Hide resolved
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/store/modules/job/actions.ts Outdated Show resolved Hide resolved
src/store/modules/job/actions.ts Outdated Show resolved Hide resolved
src/store/modules/job/actions.ts Outdated Show resolved Hide resolved
… added locales in en.json and added message in UI for no jobs found (#32j3gu0)
… added locales in en.json and added message in UI for no jobs found (#32j3gu0)
.env.example Outdated Show resolved Hide resolved
job.runtimeData && Object.keys(job.runtimeData).map((key: any) => {
if (job.runtimeData[key] === 'null') job.runtimeData[key] = ''
})
jobParams.push({ ...job.runtimeData, ...params, jobId: job.jobId });
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not pass the jobId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated sir, now passing systemJobEnumId, as discussed.

src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
src/views/BulkEditor.vue Outdated Show resolved Hide resolved
src/views/BulkEditor.vue Outdated Show resolved Hide resolved
Comment on lines 786 to 794

if(freqType) {
payload.freqType = freqType;
if(freqType === 'slow') showToast(translate("This job has slow frequency type, hence, feasible frequency will be set automatically"))
}
// TODO: find a better solution instead of hardcoding 'EVERYDAY'
payload.setTime = state.bulk.setTime;
payload.frequency = (state.bulk.frequency && freqType === 'slow') ? 'EVERYDAY' : state.bulk.frequency;
commit(types.JOB_ADDED_TO_BULK, payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(freqType) {
payload.freqType = freqType;
if(freqType === 'slow') showToast(translate("This job has slow frequency type, hence, feasible frequency will be set automatically"))
}
// TODO: find a better solution instead of hardcoding 'EVERYDAY'
payload.setTime = state.bulk.setTime;
payload.frequency = (state.bulk.frequency && freqType === 'slow') ? 'EVERYDAY' : state.bulk.frequency;
commit(types.JOB_ADDED_TO_BULK, payload);
payload.frequency = state.bulk.frequency;
if(freqType) {
payload.freqType = freqType;
if(freqType === 'slow') {
// TODO: find a better solution instead of hardcoding 'EVERYDAY'
payload.frequency = 'EVERYDAY' ;
showToast(translate("This job has slow frequency type, hence, feasible frequency will be set automatically"))
}
}
// TODO: find a better solution instead of hardcoding 'EVERYDAY'
payload.setTime = state.bulk.setTime;
commit(types.JOB_ADDED_TO_BULK, payload);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sir, the code I wrote takes into account that if the frequency is set globally, then only we override it for jobs with slow freqType (payload.frequency = (state.bulk.frequency && freqType === 'slow') ? 'EVERYDAY' : state.bulk.frequency;).

The suggested changes do not provide this functionality.

}
// TODO: find a better solution instead of hardcoding 'EVERYDAY'
payload.setTime = state.bulk.setTime;
payload.frequency = (state.bulk.frequency && freqType === 'slow') ? 'EVERYDAY' : state.bulk.frequency;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could implement a method that returns the feasible frequency based upon freqType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done sir

// As of now, the job will get scheduled again even if it is pending
const jobParams = [] as any;
let failedJobs = 0;
payload.shopifyConfigs.map((shopId: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use reduce or could handle the returned object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

return Promise.reject(resp);
}
})).then((resps: any) => {
resps.some((resp: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might fail if the request other than first one fails

Copy link
Contributor Author

@alsoK2maan alsoK2maan Jan 6, 2023

Choose a reason for hiding this comment

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

Yes, sir, updated the code to handle the case when jobs other than the first one fail.

setBulkJobData({ commit, state }, payload) {
let bulkJobs = JSON.parse(JSON.stringify(state.bulk.jobs));
const value = payload.value, type = payload.type;
if (payload.global) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could have separate actions for bulk and configuration component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done sir

k2maan and others added 7 commits January 5, 2023 12:37
…map method to create params to send in scheduleJob service which can be improved by using array reduce method, as arrayMap always returns an array whereas reduce can be used to return single element which can be obejct or anything(#32j3gu0)
src/components/JobConfigurationForBulkScheduler.vue Outdated Show resolved Hide resolved
const jobParams = [] as any;
let failedJobs = 0;
payload.shopifyConfigs.map((shopId: string) => {
return payload.jobs.reduce((params: any, job: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not using reduce correct way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated sir

}, {})
})

Promise.allSettled(jobParams.map(async (param: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use async await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sir, the use of Promise.all will reject the next calls if a previous one fails.

const resp: any = await JobService.scheduleJob(param);
if(resp.status === 200 && !hasError(resp)){
// Removing the scheduled job
dispatch('removeBulkJob', param.systemJobEnumId);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove them in bulk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated sir

src/store/modules/job/actions.ts Show resolved Hide resolved
// set the maximum slow frequency for slow type jobs if global frequnecy is set
payload.frequency = (state.bulk.frequency && freqType === 'slow') ? (generateFrequencyOptions('slow') as any).pop().value : state.bulk.frequency;

const bulk = JSON.parse(JSON.stringify(state.bulk.jobs));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const bulk = JSON.parse(JSON.stringify(state.bulk.jobs));
const bulkJobs = JSON.parse(JSON.stringify(state.bulk.jobs));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

if(job.freqType === 'slow') {
showToast(translate("Some jobs have slow frequency type, hence, feasible frequency will be set automatically"))
// If user sets a valid slow frequency, we honour it else maximum frequency is set
const slowFreqs = generateFrequencyOptions('slow').map((obj: any) => obj.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could generate them once and use further

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated sir

@adityasharma7 adityasharma7 merged commit 1197c49 into hotwax:main Jan 18, 2023
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.

Implement Bulk scheduling
6 participants