-
Notifications
You must be signed in to change notification settings - Fork 0
Leantime data provider changed to use APIData plugin instead of built in API. #242
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
base: develop
Are you sure you want to change the base?
Conversation
src/Command/SyncCommand.php
Outdated
| protected function configure(): void | ||
| { | ||
| $this->addOption("job", 'j', InputOption::VALUE_NONE, "Use async job handling"); | ||
| $this->addOption("modified", "m", InputOption::VALUE_NONE, "Only items modified since last update"); |
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.
modified implies Datetime as value, should maybe be only-modifed tom imply Bool
src/Command/SyncCommand.php
Outdated
|
|
||
| $io->info('Processing projects'); | ||
| $jobHandling = $input->getOption("job"); | ||
| $modified = $input->getOption("modified"); |
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.
As above, modified implies Datetime as value, should maybe be only-modifed tom imply Bool
src/Entity/Trait/FetchDateTrait.php
Outdated
| trait FetchDateTrait | ||
| { | ||
| #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] | ||
| private ?\DateTimeInterface $fetchTime = null; |
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.
When can this be null? For data that is sync'ed from a data provider we should always have a date.
For legacy data we can either set 1970-01-01 00:00 or NOW() as default value in the db migration. For new entities we can set new DatetomeImmutable() as default in the constructor.
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.
Not allowing null values will also simplify some of the queries in the repository methods.
| $message->modified, | ||
| ); | ||
| } catch (\Exception $e) { | ||
| throw new UnrecoverableMessageHandlingException($e->getMessage()); |
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.
Question - should we also do $this->logger->error(..)?
I think throwing UnrecoverableMessageHandlingException will put the message in the failed queue, so the error is visible there. But even so, if somebody just logs at the logs when debugging ad don't see errors it might be confusing.
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.
Added
src/Entity/Trait/FetchDateTrait.php
Outdated
|
|
||
| trait FetchDateTrait | ||
| { | ||
| #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] |
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.
Given the queries we do for oldest fetch time we should consider an index on 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.
I have changed approach to not use fetchDate to find modifiedAfter.
src/Service/JiraApiService.php
Outdated
| use Symfony\Contracts\HttpClient\HttpClientInterface; | ||
|
|
||
| class JiraApiService implements DataProviderServiceInterface | ||
| class JiraApiService implements DataProviderInterface |
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.
| class JiraApiService implements DataProviderInterface | |
| #[\Deprecated(message: "The Jira data provider is deprcated and will be removed", since: "x.y")] | |
| class JiraApiService implements DataProviderInterface |
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.
I remove the Service as it does not need to exist with the changes in this PR and it does not work.
src/Service/DataProviderService.php
Outdated
|
|
||
| private function linkToTicket(string $ticketId, DataProvider $dataProvider): string | ||
| { | ||
| return $dataProvider->getUrl() . "/errorpage/#/tickets/showTicket/" . $ticketId; |
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.
This is technically only valid for Leantime, so does not belong in a generic data provider class. But I don't know how important we consider the support for multiple providers to be going forward?
src/Model/Upsert/UpsertIssueData.php
Outdated
|
|
||
| use App\Enum\IssueStatusEnum; | ||
|
|
||
| class UpsertIssueData |
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.
I Don't see the point of having Upsert in the class name for any of these models, but that is semantics and subjective
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.
I will leave it as it ensures that the name does not collide with other IssueData class.
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.
What about ProviderIssueDataas we talked about?
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.
Forgot about that talk :D
src/Repository/IssueRepository.php
Outdated
| return $qb->getQuery()->getSingleColumnResult(); | ||
| } | ||
|
|
||
| public function getOldestFetchTime(DataProvider $dataProvider, ?array $projectTrackerProjectIds = null): ?\DateTimeInterface |
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.
I would expect this to be getNewestFetchTime? If we always fetch from the oldest and forward will we not always sync everything. Or do I have it the wrong way round?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #242 +/- ##
==========================================
Coverage ? 17.38%
Complexity ? 1662
==========================================
Files ? 202
Lines ? 6880
Branches ? 0
==========================================
Hits ? 1196
Misses ? 5684
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
turegjorup
left a comment
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.
Consider if we can lower this in docker-compose.server.override.yml
phpfpm:
environment:
- PHP_MEMORY_LIMIT=512M
src/Command/SyncDeletedCommand.php
Outdated
| name: 'app:data-providers:sync-deleted', | ||
| description: 'Sync Data Provider deleted data, that has been deleted within the last hour, as jobs. Scheduled to run every 15 minutes.', | ||
| )] | ||
| #[AsPeriodicTask(frequency: '15 minutes')] |
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.
I havn't tried it, but it should be possible to do something like #[SomeAttribute('%env(MY_ENV_VAR)%')], so
| #[AsPeriodicTask(frequency: '15 minutes')] | |
| #[AsPeriodicTask(frequency: '%env(SYNC_FREQUENCY)%')] |
And then set SYNC_FREQUENCY='15 minutes' in .env so it's configurable per installation
src/Command/SyncDeletedCommand.php
Outdated
| { | ||
| // Look at entries modified within the last hour. | ||
| $deletedAfter = new \DateTime(); | ||
| $deletedAfter->sub(new \DateInterval('P1D')); |
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.
P1Dis one DAY, but comment and description says one HOUR. I'm fine with either, as long as code and comment/description match.
Nice to have: P1D as default, but possible to give another value as command argument.
src/Command/SyncModifiedCommand.php
Outdated
| name: 'app:data-providers:sync-modified', | ||
| description: 'Sync Data Provider data, that has been modified within the last hour, as jobs. Scheduled to run every 15 minutes.', | ||
| )] | ||
| #[AsPeriodicTask(frequency: '15 minutes')] |
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.
As above. If we can move 15 minutes to .env we should
| #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] | ||
| private ?\DateTimeInterface $fetchDate = null; | ||
|
|
||
| #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] | ||
| private ?\DateTimeInterface $sourceModifiedDate = null; | ||
|
|
||
| public function getFetchDate(): ?\DateTimeInterface | ||
| { | ||
| return $this->fetchDate; | ||
| } | ||
|
|
||
| public function setFetchDate(?\DateTimeInterface $fetchDate): void | ||
| { | ||
| $this->fetchDate = $fetchDate; | ||
| } | ||
|
|
||
| public function getSourceModifiedDate(): ?\DateTimeInterface | ||
| { | ||
| return $this->sourceModifiedDate; | ||
| } | ||
|
|
||
| public function setSourceModifiedDate(?\DateTimeInterface $sourceModifiedDate): void | ||
| { | ||
| $this->sourceModifiedDate = $sourceModifiedDate; | ||
| } |
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.
I'm torn between wanting to use an interface and wanting to use DateTimeImmutable (which has no interface). I would do this, but it's only a suggestion. Pick the one you prefer.
| #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] | |
| private ?\DateTimeInterface $fetchDate = null; | |
| #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] | |
| private ?\DateTimeInterface $sourceModifiedDate = null; | |
| public function getFetchDate(): ?\DateTimeInterface | |
| { | |
| return $this->fetchDate; | |
| } | |
| public function setFetchDate(?\DateTimeInterface $fetchDate): void | |
| { | |
| $this->fetchDate = $fetchDate; | |
| } | |
| public function getSourceModifiedDate(): ?\DateTimeInterface | |
| { | |
| return $this->sourceModifiedDate; | |
| } | |
| public function setSourceModifiedDate(?\DateTimeInterface $sourceModifiedDate): void | |
| { | |
| $this->sourceModifiedDate = $sourceModifiedDate; | |
| } | |
| #[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: true)] | |
| private ?\DateTimeImmutable $fetchDate = null; | |
| #[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: true)] | |
| private ?\DateTimeImmutable $sourceModifiedDate = null; | |
| public function getFetchDate(): ?\DateTimeImmutable | |
| { | |
| return $this->fetchDate; | |
| } | |
| public function setFetchDate(?\DateTimeInterface $fetchDate): void | |
| { | |
| if (!$fetchDate instanceof \DateTimeImmutable) { | |
| $fetchDate = \DateTimeImmutable::createFromInterface($fetchDate); | |
| } | |
| $this->fetchDate = $fetchDate; | |
| } | |
| public function getSourceModifiedDate(): ?\DateTimeImmutable | |
| { | |
| return $this->sourceModifiedDate; | |
| } | |
| public function setSourceModifiedDate(?\DateTimeImmutable $sourceModifiedDate): void | |
| { | |
| $this->sourceModifiedDate = $sourceModifiedDate; | |
| } |
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.
I think I will stick with DateTimeInterface, since this is the approach we use in the other parts of the codebase.
| } | ||
|
|
||
| return new JsonResponse(['issuesSynced' => $issuesSynced], 200); | ||
| return new JsonResponse(['issuesSynced' => 0], 200); |
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.
Does it make sense to return 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.
Not really... will remove it :)
I have removed the override value. We will have to see it we need more than 128M |
641257b to
22bbd84
Compare
…ger exists in the source
4145988 to
013fa32
Compare
Link to ticket
https://leantime.itkdev.dk/TimeTable/TimeTable?showTicketModal=5538#/tickets/showTicket/5538
Description
TODOs
Checklist