Skip to content

Conversation

@shabab477
Copy link
Contributor

What does it do?

I have added cron timezone configuration on the cron initialisation hook. Strapi uses node-scheduler, the way you add timezone with this library is mentioned here. I implemented the timezone feature in a similar manner

Why is it needed?

As mentioned in 3099

Related issue(s)/PR(s)

None

@shabab477 shabab477 requested a review from a team October 17, 2020 15:37
@shabab477 shabab477 force-pushed the feature/add-cron-timezone branch from 8c859c4 to d76606f Compare October 17, 2020 15:39
@codecov
Copy link

codecov bot commented Oct 17, 2020

Codecov Report

Merging #8373 (c6376f7) into master (f6558c7) will increase coverage by 0.05%.
The diff coverage is 76.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8373      +/-   ##
==========================================
+ Coverage   33.19%   33.25%   +0.05%     
==========================================
  Files        1220     1221       +1     
  Lines       13618    13636      +18     
  Branches     1357     1359       +2     
==========================================
+ Hits         4521     4535      +14     
- Misses       8212     8217       +5     
+ Partials      885      884       -1     
Flag Coverage Δ
front 24.70% <0.00%> (-0.02%) ⬇️
unit 54.63% <84.61%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/strapi-admin/services/permission.js 71.64% <ø> (-5.93%) ⬇️
...nager/admin/src/components/PreviewWysiwyg/index.js 0.00% <0.00%> (ø)
...rc/components/PreviewWysiwyg/utils/satinizeHtml.js 0.00% <0.00%> (ø)
.../src/containers/InputModalStepperProvider/index.js 0.00% <0.00%> (ø)
...-upload/admin/src/containers/ModalStepper/index.js 0.00% <0.00%> (ø)
...rapi-plugin-upload/admin/src/translations/index.js 0.00% <0.00%> (ø)
packages/strapi-utils/lib/models.js 24.69% <25.00%> (-0.31%) ⬇️
packages/strapi-admin/services/role.js 96.99% <98.00%> (+2.67%) ⬆️
packages/strapi-admin/controllers/role.js 47.16% <100.00%> (ø)
packages/strapi-admin/services/metrics.js 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fad9fda...2d25b1d. Read the comment docs.

@shabab477 shabab477 force-pushed the feature/add-cron-timezone branch from d76606f to d4aaec3 Compare October 18, 2020 00:53
@shabab477 shabab477 requested review from derrickmehaffy and removed request for a team October 18, 2020 00:56
@shabab477 shabab477 mentioned this pull request Oct 19, 2020
2 tasks
@derrickmehaffy derrickmehaffy requested a review from a team October 19, 2020 14:54
derrickmehaffy
derrickmehaffy previously approved these changes Oct 19, 2020
Copy link
Member

@derrickmehaffy derrickmehaffy 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 still need eng team review

@MattieBelt
Copy link
Collaborator

Nice, updating the docs would also be nice :)

@shabab477
Copy link
Contributor Author

shabab477 commented Oct 24, 2020

@MattieBelt do you recommend doing this in this PR or in a separate PR? The contributing guide says documentation fixes should point to the documentation branch. Or I am taking the guide way too literally.

@MattieBelt
Copy link
Collaborator

Better to keep with the feature, so adding it in this PR would better.
Also the docs branch will be merged with master when Strapi has a new release. (right? @derrickmehaffy )

@derrickmehaffy
Copy link
Member

Better to keep with the feature, so adding it in this PR would better.
Also the docs branch will be merged with master when Strapi has a new release. (right? @derrickmehaffy )

Yes, keep it with this pr, we will handle the merge of documentation. The contribution guide in that case is only for doc fixes

@derrickmehaffy derrickmehaffy self-requested a review October 26, 2020 14:00
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

I think we should support object cron to be able to configure each cron as it work in the lib instead of adding global options.

@lauriejim
Copy link
Contributor

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/strapi-cron-jobs-just-for-production-environment/715/2

@shabab477
Copy link
Contributor Author

shabab477 commented Oct 26, 2020

I think we should support object cron to be able to configure each cron as it work in the lib instead of adding global options.

@alexandrebodin changed code allows both the following snippets

'*/1 * * * *': () => {
    console.log('1 minute later');
},

The change you're asking for probably needs a change like:

'*/1 * * * *': {
  task: () => {
     console.log('1 minute later');
  },
  tz: 'Asia/Dhaka'
}

I am not sure whether I have over engineered this though.

@MattieBelt Added documentation

@shabab477
Copy link
Contributor Author

Can I get a hacktoberfest-accepted label though? I've contributed to other repos, seems like they're not available for the PR-s.

@derrickmehaffy
Copy link
Member

Can I get a hacktoberfest-accepted label though? I've contributed to other repos, seems like they're not available for the PR-s.

No need unless DigitalOcean has changed something that we are unaware of. So long as code is merged into our master branch and it doesn't have our spam label it should count. If something has changed can you point us towards the documentation for it?

@shabab477
Copy link
Contributor Author

Yes if it gets merged before 30th then it counts otherwise it doesn't. I don't wanna hurry this PR though, better if we take time and go with the best possible solution.

@shabab477 shabab477 requested review from alexandrebodin and removed request for a team November 2, 2020 16:30
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

We could make it a be more flexible by having;

{
  '* * * * *': {
   task() {},
   options: {
    tz: '',
    // other possible options
   } 
  }
}

@shabab477 shabab477 force-pushed the feature/add-cron-timezone branch from c91f5ba to 412e389 Compare November 4, 2020 15:48
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

almost there ;)

Signed-off-by: Shabab Karim <shababkarim93@gmail.com>
@shabab477 shabab477 force-pushed the feature/add-cron-timezone branch from 412e389 to 75c3ff9 Compare November 8, 2020 15:21
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM, Refactored just a bit to make it simpler ;) Thansk for you contribution !

@alexandrebodin alexandrebodin added this to the 3.3.0 milestone Nov 9, 2020
@alexandrebodin alexandrebodin merged commit 12284b9 into strapi:master Nov 9, 2020
@alexandrebodin alexandrebodin added source: core:strapi Source is core/strapi package issue: enhancement Issue suggesting an enhancement to an existing feature labels Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

issue: enhancement Issue suggesting an enhancement to an existing feature source: core:strapi Source is core/strapi package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants