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

Workflows #1378

Merged
merged 193 commits into from
Apr 20, 2018
Merged

Workflows #1378

merged 193 commits into from
Apr 20, 2018

Conversation

sfmskywalker
Copy link
Member

This PR adds the new Workflows module.

Jetski5822 and others added 30 commits July 21, 2017 20:23
# Conflicts:
#	Orchard.sln
#	src/OrchardCore.Modules/Orchard.Demo/Middlewares/BlockingMiddleware.cs
#	src/OrchardCore.Modules/Orchard.Demo/Views/Home/Index.cshtml
#	src/OrchardCore.Modules/Orchard.Mvc/Startup.cs
#	src/OrchardCore.Modules/Orchard.Resources/ResourceManifest.cs
#	src/OrchardCore.Modules/Orchard.Setup/Recipes/cms.recipe.json
#	src/OrchardCore.Modules/Orchard.Widgets/Controllers/AdminController.cs
#	src/OrchardCore/Orchard.DisplayManagement/Notify/NotifyFilter.cs
#	src/OrchardCore/Orchard.Mvc.Core/Extensions/ModularServiceCollectionExtensions.cs
#	src/OrchardCore/Orchard.ResourceManagement/ResourceManager.cs
#	src/OrchardCore/OrchardCore.Cms/OrchardCore.Cms.csproj
Sipke Schoorstra added 3 commits April 14, 2018 13:25
# Conflicts:
#	OrchardCore.sln
#	src/OrchardCore.Modules/OrchardCore.OpenId/Drivers/OpenIdServerSettingsDisplayDriver.cs
IClock clock,
IStringLocalizer<SecurityTokenService> localizer)
{
_dataProtector = dataProtectionProvider.CreateProtector("Tokens").ToTimeLimitedDataProtector();
Copy link
Member

Choose a reason for hiding this comment

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

Note: now that you merged the dev branch into your branch, you'll probably want to add a module dependency on OrchardCore.DataProtection to ensure the IDataProtectionProvider instance you get is tenant-isolated (and not provided by the host).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand. If Workflows should use the tenant-isolated service, then shouldn't all modules using data protection use the tenant-isolated one?

Copy link
Member

Choose a reason for hiding this comment

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

I assume you get an instance from an indirect reference to a module that requires it, but you require it too and you should declare a dependency on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see now. Thanks, will fix.

@jtkech
Copy link
Member

jtkech commented Apr 14, 2018

Have you tried */1 * * * *? That expression triggers every 1 minute.

Yes i tried it and it triggers every 1 minute. So, if your 1st reference time is 2min:01sec it will trigger at 3min:00 and then at 4min:00 and so on. But if your 1st reference time is e.g 2min:59sec, the first occurence is still 3min:00 (only one second after).

@sfmskywalker
Copy link
Member Author

Ah now I see what you mean. That does behave differently than what one might expect when using a Timer activity. So perhaps I should rename it to Cron, and reintroduce a Timer activity that does more or less exactly what one might expect: trigger every X interval from the point where the service started. One concern though is that the difference between the two might be too subtle. What do you think about this?

@jtkech
Copy link
Member

jtkech commented Apr 15, 2018

Hmm, just an idea, what you could do is to get the next occurence, reinit your reference to this occurence and get again the next occurence, so that you can compute the time span between 2 occurences. Then you will have to wait from now to now + this time span.

Maybe we could have an option to use the cron expression as a time interval.

  • If unchecked : The timer will trigger on the next occurence defined by the cron expression and following the time the timer has been started. Note: the timer could trigger immediately.

  • If checked : use the cron expression as a time interval, so that when started the timer will trigger after this time interval. The time interval being the time between 2 occurences as defined by the cron expression.

Note: knowing that right now the granularity of bg tasks is 1 minute.

But not sure it is clearer ;) just an idea.

@sfmskywalker
Copy link
Member Author

That's very clever, I like it!

@sfmskywalker
Copy link
Member Author

@sebastienros
Copy link
Member

I wanted to merge it, then the merge is conflicting (with theme changes) Someone fixes it and merge please.

@sfmskywalker
Copy link
Member Author

I'll fix.

# Conflicts:
#	src/OrchardCore.Themes/TheAdmin/Assets/scss/TheAdmin.scss
#	src/OrchardCore.Themes/TheAdmin/wwwroot/Styles/TheAdmin.css
@sfmskywalker sfmskywalker merged commit 44754f6 into dev Apr 20, 2018
@sfmskywalker sfmskywalker deleted the workflows branch April 20, 2018 12:00
@sfmskywalker sfmskywalker restored the workflows branch April 20, 2018 12:00
@sfmskywalker sfmskywalker deleted the workflows branch April 20, 2018 12:00
@kevinchalet
Copy link
Member

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.

6 participants