@diosmosis opened this Pull Request on May 26th 2020 Member

Refs #15117

@tsteur commented on May 27th 2020 Member

@diosmosis now getting an exception isPeriodInThisPeriod method not defined

@diosmosis commented on May 27th 2020 Member

Oh, huh, I forgot to implement that, but the tests were passing for me... interesting, will fix.

@diosmosis commented on May 27th 2020 Member

@tsteur pushed

@diosmosis commented on May 27th 2020 Member

Also not sure if I missed it or if this is covered by a lock. Should the very same archive already be in process (same site, period, date and segment (if given)) to then skip as well?

Covered by two things, first is that if the archive is DONE_IN_PROGRESS we shouldn't handle it. Second is if it is locked and not expired, we shouldn't handle it. We could also just do <= when checking period ID and that would also prevent it in a third check. I'll just add that for fun.

@diosmosis commented on May 27th 2020 Member

FYI, I'm planning on refactoring/testing all of CronArchive in #11974

@tsteur commented on May 27th 2020 Member

@diosmosis don't have to add it unless it's easy. The DONE IN PROGRESS should be fine too

@diosmosis commented on May 27th 2020 Member

@tsteur single character change, so definitely easy

@tsteur commented on May 27th 2020 Member

👍 feel free to change it and I have another look

@diosmosis commented on May 27th 2020 Member

@tsteur you can take another look

@tsteur commented on May 27th 2020 Member

@diosmosis some tests are failing now. Otherwise looks good to merge

This Pull Request was closed on June 2nd 2020
Powered by GitHub Issue Mirror