-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Archive Later functionality (Lombiq Technologies: OCORE-72) #10935
Conversation
I will try to add unit tests if requires. Refactoring the common APIs with PublishLater module could goes into another PR |
Strange, that VS tell that the cast is unnecessary, but the build fails here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but we are still waiting for the background job infrastructure to support this correctly.
Fortunately I am in the middle of working on it, so this should wait until it is complete, as it will change and improve how small jobs like this can be scheduled.
I already started another PR in my local machine, I tried to add @deanmarcussen could you please outline what you are planning to do to avoid duplicate the work, regarding this PR we could merge then make both ArchiveLater & PublishLater build on top of the new infrastructure |
If you haven't done it already, please also check if the button styling is OK if Publish Later is also enabled (or if there are any clashes by chance). |
Seems I saw something similar when I introduced a content scheduling APIs, I will double check if this happen if both modules enables. Thanks for this reminder |
@deanmarcussen any update on the work that you done in the infrastructure APIs, it will be good to share WIP PR if its possible |
It was demoed on Tuesday. You can see the video when it is on youtube. |
Oh really, I will check the Lombiq channel if the video is posted |
Since Dean is not replying under #5755, let's merge this. For that, please resolve the merge conflict, Hisham, then I'll review. |
Just for info some quick thoughts (so need to be checked) that may also concern the PublishLater. Sorry for the long comment as usual ;) but most of them are only descriptions. Maybe in place of doing a joined I saw that you set the Utc to null in an handler both for PublishLater and ArchiveLater, but I think it is better to use the Updated: Okay, normally on soft delete the index provider remove it from the index table. So maybe filter the query on Latest and not Published for publishLater, and Latest and Published for ArchiveLater. The logic needs to be checked but the goal is to prevent some use cases where items would always match forever, and also to have a better bounded query. And if we use a direct Finally about the But if we use a For example
. |
Hisham, could you complete this, together with JT's remarks, please? |
Sure, hope I find time tonight to read @jtkech comments and apply the requested changes |
BTW I did a prototype for content scheduling APIs that may I share it later |
Do you mean a notification after saving? Yeah, that looks useful. |
I didn't mean the notification, there's a label show up even if the item has been archived, I'm not sure if it's act as indicator to the admins, but still I don't see any value if the item is archived |
Please show a screenshot of what you mean because I don't understand. |
@if (Model.ScheduledArchiveUtc.HasValue) | ||
{ | ||
<div> | ||
<span class="hint">@T["Scheduled to be archived on {0}", (object)(await DisplayAsync(await New.DateTime(Utc: Model.ScheduledArchiveUtc)))]</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Piedone I mean this hint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that we shouldn't display this once ScheduledArchiveUtc
passed? That's a good idea, yes, in that case we should hide this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I'm still waiting for Jean to prove the latest changes, then I can hide the hint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if we still set the date to null when the item is archived, nothing to do here
But it depends on what we may want, see my last comment in the main thread.
Sorry didn't have so much time but after a quick look most of things look good to me. I think we should still set
If we should set the date to null in the both above cases we should just keep the handler, if only for the first case one solution would be to set the date to null in the bg task itself (easy to do). What you think about doing the same tweaks but for PublishLater, in a separate PR I think, the more sensitive part would be a migration step, normally no problem to add columns but for the |
I don't really see much appeal in making things more complex for some edge cases. I don't really understand how the index you mention relates to all this. |
@Piedone Sorry if I was not clear, here a summary Here do we need to call again the handler to set the archive date to null, or just set it to null in the bg task itself, or not at all, it may depend on the above 1. and 2. in my previous comment, I let you choose. Then do we need to do the same kind of tweaks for PublishLater, e.g. add Latest and Published to the index and use them, which would imply to add a migration step. |
Ah, I see. Yes, we should then go back to the Publish Later modules indeed. |
I already created another PR after I made the update on this, please check #12276 |
Just for the fun another use case, if we use both PublishLater and ArchiveLater, if we don't set the schedule date to null, an item could be Published then Unpublished then Published ... every minute. So looks good to reset both states that triggered the bg Task action, including the schedule date, for example in the bg Task itself, I will add a review comment. |
src/OrchardCore.Modules/OrchardCore.ArchiveLater/Handlers/ArchiveLaterPartHandler.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.ArchiveLater/Indexes/ArchiveLaterPartIndex.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.ArchiveLater/Services/ScheduledArchivingBackgroundTask.cs
Show resolved
Hide resolved
I'm not sure how it will be a problematic, coz we already filter the items in the service, let me think |
Just for info Yes this is confusing, we have Index Tables that we used for joined queries and that are regular SQL tables, then each index table, can have itself inner indexes, as you can see e.g. under Sql Mangement Studio if you expand the index table under its dedicated These inner indexes are not required but we add non clustered ones for performance only, those that are named That's why we have |
@Piedone can we file an issue descriping the background job infrastructure features list? |
Please do. |
I think we need @jtkech help on this, to mention what we should offer in such infrastructure? How should they different from background tasks we have BTW I remembered I started a prototype long time back JUST to simplify the scheduling for both Archive & Publish Later modules |
May be we already have a rich info here |
Fixes #5754