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

Komga implementation #943

Merged
merged 70 commits into from
Aug 20, 2024
Merged

Komga implementation #943

merged 70 commits into from
Aug 20, 2024

Conversation

Jacob-Tate
Copy link
Contributor

This appears to be working well in my testing and properly handles mass marking as read as well as marking in progress reading correctly. Needed to add in a special case in misc for a progress update where the chapter is provided out of order as occasionally apps like mihon appear to update this information in random order. This occasionally resulted in the wrong chapter being marked as read.

Cleaned up where some variables lived and documented important code.

Removed unused imports

Templatized duplicated code.

Switched to an unbounded channel to allow users to make any length of
background task polling periods.
@IgnisDa
Copy link
Owner

IgnisDa commented Aug 5, 2024

Looking good! Requested for a few small changes. Will take a better look when I get the time. Thanks!

@Jacob-Tate
Copy link
Contributor Author

Let me know if I need to make any changes here.

apps/backend/Cargo.toml Outdated Show resolved Hide resolved
apps/backend/src/background.rs Outdated Show resolved Hide resolved
apps/backend/src/entities/integration.rs Outdated Show resolved Hide resolved
@Jacob-Tate
Copy link
Contributor Author

Let me know if there are any other concerns.

docs/content/integrations.md Outdated Show resolved Hide resolved
docs/content/integrations.md Outdated Show resolved Hide resolved
@Jacob-Tate
Copy link
Contributor Author

Addressed a bug where some manga imported with a different number than is shown in the interface not sure what causes this but metadata.number is the authoritative number and is also the one that gets edited in the interface the original item.number appears to be the order in which they import into the series.

@IgnisDa
Copy link
Owner

IgnisDa commented Aug 9, 2024

Not sure about this, but is your rust formatter working? Maybe you have to install rust-analyzer. None of the rust file changes you made are formatted, it seems.

@IgnisDa
Copy link
Owner

IgnisDa commented Aug 9, 2024

The pro version also supports "Sync to Owned collection" boolean, will this be something this integration can support?

image

@Jacob-Tate
Copy link
Contributor Author

It appears my IDE had the formatter disabled I believe this has been resolved.

@Jacob-Tate
Copy link
Contributor Author

Jacob-Tate commented Aug 9, 2024

The pro version also supports "Sync to Owned collection" boolean, will this be something this integration can support?

How does this functionality work? Does it just add the things which are tracked to the owned collection or is this doing an import of all books into the owned collect?

@IgnisDa
Copy link
Owner

IgnisDa commented Aug 9, 2024

As of now, it just works for ABS. It just adds all items in your library to the owned collection. Can I give you access to the Pro repo, and you can take a look at the code?

@Jacob-Tate
Copy link
Contributor Author

Yeah that works I see no reason why we couldnt support that

@IgnisDa
Copy link
Owner

IgnisDa commented Aug 9, 2024

Sent you the invitation. This PR LGTM, i will merge it later today.

should-run: ${{ steps.check.outputs.should-run }}
should-release: ${{ steps.tag.outputs.should-release }}
should-run: ${{ steps.set_outputs.outputs.should-run }}
image-names: ${{ steps.set_outputs.outputs.image-names }}
Copy link
Owner

Choose a reason for hiding this comment

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

@vnghia I have been trying to get this working for some time, but it seems that there is no way to publish a docker image from a PR (from a fork) since the GITHUB_TOKEN granted to the workflow only has read permissions (despite specifying write explicitly in the action).

Do you know a solution to this?

Copy link
Owner

Choose a reason for hiding this comment

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

So the current flow never works for PRs. Only for PRs from a branch in the same repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

let me think about it. you can use env instead of secrets but it will print your env in the log

Copy link
Owner

Choose a reason for hiding this comment

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

But to actually inject the secrets into env, i would still need access to secrets. Which are not available in PRs from forks.

@Jacob-Tate Jacob-Tate marked this pull request as draft August 18, 2024 05:34
Introduced YankIntegrationWithCommit trait to address issue with passing the closure through a staticly allocated enum.
Renamed the integration file to integration trait and moved all integrations traits to this file
Changed visibility of all integrations to be pub(crate) since they dont need to be accessible anywhere else.
Introduced proper error handling for improper integration types
@Jacob-Tate Jacob-Tate marked this pull request as ready for review August 18, 2024 06:13
@IgnisDa
Copy link
Owner

IgnisDa commented Aug 19, 2024

@Jacob-Tate Is this ready to be merged?

@Jacob-Tate
Copy link
Contributor Author

Yes this should be ready ive been running it for a few days without any issues.

@IgnisDa IgnisDa merged commit f537c10 into IgnisDa:main Aug 20, 2024
9 checks passed
@IgnisDa
Copy link
Owner

IgnisDa commented Aug 20, 2024

Will release this with v7 next week. Thanks for the contribution!

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.

3 participants