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

feat: Refactor form sync to run as a background job with retry #2408

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

AIlkiv
Copy link
Collaborator

@AIlkiv AIlkiv commented Nov 13, 2024

issue: #2165 , #2067

I implemented a logic to synchronize the form with the file, allowing multiple attempts if the first one fails. The process runs in the background with delays, ensuring it does not impact the user experience.

@AIlkiv AIlkiv force-pushed the form-sync-background-job branch 2 times, most recently from e08797f to 493c3fe Compare November 13, 2024 15:54
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@dc2237e). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2408   +/-   ##
=======================================
  Coverage        ?   44.27%           
  Complexity      ?      860           
=======================================
  Files           ?       73           
  Lines           ?     3268           
  Branches        ?        0           
=======================================
  Hits            ?     1447           
  Misses          ?     1821           
  Partials        ?        0           
---- 🚨 Try these New Features:

@AIlkiv AIlkiv requested review from Chartman123 and Koc November 13, 2024 16:00
@AIlkiv AIlkiv force-pushed the form-sync-background-job branch from 493c3fe to d34796b Compare November 14, 2024 01:56
@AIlkiv AIlkiv changed the title Refactor form sync to run as a background job with retry feat: Refactor form sync to run as a background job with retry Nov 14, 2024
@AIlkiv
Copy link
Collaborator Author

AIlkiv commented Nov 14, 2024

Added a fix for the issue described in #2067

@AIlkiv AIlkiv force-pushed the form-sync-background-job branch from d34796b to bc6bcab Compare November 14, 2024 04:08
@AIlkiv AIlkiv requested a review from susnux November 14, 2024 04:09
@Chartman123 Chartman123 added bug Something isn't working php PHP related ticket 3. to review Waiting for reviews labels Nov 14, 2024
@Chartman123 Chartman123 added this to the 5.0 milestone Nov 14, 2024
Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Looks good to me :) I'd like to wait with the merging for @susnux's review

thanks for this PR, we can also do a backport to stable4, but you'll need to adjust the ApiController part there manually because of the duplicated routes.

@Chartman123
Copy link
Collaborator

/backport to stable4

@backportbot backportbot bot added the backport-request Pending backport by the backport-bot label Nov 14, 2024
@Chartman123
Copy link
Collaborator

The Codecov upload error isn't specific to Forms: codecov/codecov-action#1643

Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

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

I am not sure we need the max attempts, but otheriwse fine with me.

Please fix the code style in the constructor

@Koc Koc force-pushed the form-sync-background-job branch 2 times, most recently from 7f49d49 to dd86b4a Compare November 19, 2024 22:52
@Koc
Copy link
Collaborator

Koc commented Nov 19, 2024

@susnux @Chartman123 I did requested changes. All pipelines are green except 2: PHPUnit SQLite + coverage upload. But tests pass

@Chartman123
Copy link
Collaborator

Might be because you made changes on a branch in a fork that doesn't belong to you. It could also help to rebase to current main

@Koc
Copy link
Collaborator

Koc commented Nov 19, 2024

Already did by using Github UI button 🤷‍♂️

@susnux
Copy link
Collaborator

susnux commented Nov 20, 2024

It is only the codecov action that is broken for forks, tests are fine

The codecov action was updated but they changed the syntax and we forgot to adjust it.
Let me fix that 🙈

@Koc Koc force-pushed the form-sync-background-job branch from dd86b4a to 10047d2 Compare November 20, 2024 17:02
@Koc
Copy link
Collaborator

Koc commented Nov 20, 2024

pipeline is completely green now ✔️

@susnux susnux merged commit 7f7b470 into nextcloud:main Nov 20, 2024
48 checks passed
Copy link

backportbot bot commented Nov 20, 2024

The backport to stable4 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable4
git pull origin stable4

# Create the new backport branch
git checkout -b backport/2408/stable4

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 10047d28

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/2408/stable4

Error: Failed to cherry pick commits: error: no cherry-pick or revert in progress
fatal: cherry-pick failed


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@Koc
Copy link
Collaborator

Koc commented Nov 20, 2024

I will take care about backport to stable4 later today

@Chartman123
Copy link
Collaborator

Nice, thank you :)

@Koc
Copy link
Collaborator

Koc commented Nov 21, 2024

here we go #2424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews backport-request Pending backport by the backport-bot bug Something isn't working php PHP related ticket
Projects
None yet
4 participants