Improve and modernize obs_rsync.js

[Lokeshsri11]

There are a few bugs that I can identify.
First, the controlToShow parameter is not passed correctly to the error callback function of the fetchValue function. It should be passed as a separate parameter instead of being included in the callback function's parameter list.

Second, the postAndRedrawElement function doesn't handle the case where the btn.dataset.geturl value is not defined. It should check if the value exists before calling the fetchValue function.

Third, the postAndRedirect function doesn't check if the redir parameter is defined before redirecting to the URL.

And here also the way to optimize our code :
1.Use let and const instead of var to declare variables. let and const provide block-level scoping, which can help avoid variable hoisting and improve code readability.

2.Use arrow functions instead of anonymous functions to improve code readability and reduce typing.

3.Use object destructuring to simplify the code when accessing object properties.

4.Use const instead of var when declaring constant values.

5.Use template literals to concatenate strings instead of using the + operator.

6.Use default function parameters to simplify function calls and provide default values for parameter

[Martchus]

Not sure whether it is required, but also make sure you run make tidy-js. Otherwise the CI checks might complain.

I personally also usually use let and const and the other JavaScript features you've mentioned and gladly accept the PR. However, we usually don't enforce using these for contributions so that's why our JavaScript code which has been touched by many different people is at it is. This is likely not going to change any time soon unless one is willing to modernize it and enforce the style. (Some of our code also simply predates the introduction of certain modern JavaScript features.)

[codecov]

Codecov Report

Merging #5068 (12066ae) into master (78c4788) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5068   +/-   ##
=======================================
  Coverage   98.25%   98.25%           
=======================================
  Files         383      383           
  Lines       36026    36026           
=======================================
+ Hits        35396    35398    +2     
+ Misses        630      628    -2     

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Martchus]

You have to run make tidy-js as the JavaScript style-check fails.

[Lokeshsri11]

Wait @Martchus i'll update my code and try to make it in tidy-js format

[Martchus]

I'm adding the "not-ready" label only to prevent mergify from merging automatically as we should squash those commits.

[Lokeshsri11]

Okay @Martchus, but I think there are no remaining issues since it passed all test cases :)

[Martchus]

Yes, we only need to wait for one more approval from the team. Then I'd merge your changes squashing them into one commit.

[kalikiana]

Nice tweaks!

[Lokeshsri11]

Thank you @Martchus @kalikiana for this PR review and I will continue to try and contribute more to this project.

[Martchus]

We have to thank you!

Note that having multiple commits with generic messages is not ideal. We normally wouldn't merge a PR in that state. Here I've been using GitHub's feature to squash the commits and I gave the resulting commit a more specific message based on the PR description. Checkout 5fdb6a4 for how the commit looks now.

It would be great if you'd follow https://cbea.ms/git-commit next time when committing changes. Then I don't have to squash them. Note that having multiple, smaller commits on your PR branch is totally fine as long as the commit messages are meaningful.

[Lokeshsri11]

Okay @Martchus , I will pay attention to all these things from now on :)