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

[For 10.4] Fix doc links in the admin settings #36315

Merged
merged 1 commit into from
Dec 3, 2019
Merged

[For 10.4] Fix doc links in the admin settings #36315

merged 1 commit into from
Dec 3, 2019

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Oct 22, 2019

TODO

DOC should have the following rewrites:

Description

Fix links in setupchecks.js

Related Issue

Motivation and Context

Proper documentation links

How Has This Been Tested?

Browse to Settings -> Admin -> General via HTTP
Check where link You are accessing this site via HTTP. We strongly suggest you configure your server to require using HTTPS instead as described in our security tips. points

Expected

to ownCloud docs

Actual

to not-existing #admin-tips anchor on the same page

The same is valid for HSTS message

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Please use url generator - see

public function linkToDocs($key);

@DeepDiver1975 DeepDiver1975 dismissed their stale review October 23, 2019 10:26

wrong assumption ... 🙈

@VicDeo
Copy link
Member Author

VicDeo commented Oct 23, 2019

@DeepDiver1975 I implemented linkToDocs so I'm aware of it ;)
But it is unavailable in JS and somebody did it another way there:
core/js/config.php produces oc_defaults.docPlaceholderUrl
and any JS script that needs it replace PLACEHOLDER with the section. Like this

var docUrl = placeholderUrl.replace('PLACEHOLDER', 'admin-setup-well-known-URL');

@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #36315 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36315      +/-   ##
============================================
+ Coverage     64.68%   64.68%   +<.01%     
  Complexity    19023    19023              
============================================
  Files          1268     1268              
  Lines         74362    74364       +2     
  Branches       1309     1311       +2     
============================================
+ Hits          48100    48102       +2     
  Misses        25876    25876              
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54.02% <100%> (+0.01%) 0 <0> (ø) ⬇️
#phpunit 65.86% <ø> (ø) 19023 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/js/setupchecks.js 93.93% <100%> (+0.12%) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5cedc1...24801b0. Read the comment docs.

@VicDeo
Copy link
Member Author

VicDeo commented Oct 25, 2019

Scality failure is irrelevant:

wait-for-it -t 60 scality:8000
services aren't ready in 1m0s

@VicDeo
Copy link
Member Author

VicDeo commented Nov 8, 2019

@micbar please triage

@mmattel
Copy link
Contributor

mmattel commented Nov 13, 2019

In any case, please DO NOT write doc links as:
https://doc.owncloud.org/server/10.3/admin_manual/configuration/server/harden_server.html#use-https

When 10.3 will be superseded, this link may go away. Please use instead the version latest.
https://doc.owncloud.org/server/latest/admin_manual/configuration/server/harden_server.html#use-https

@phil-davis
Copy link
Contributor

@micbar is this scheduled for more review then release or?

@micbar
Copy link
Contributor

micbar commented Dec 2, 2019

@VicDeo Could you take care to get it merged?

@VicDeo
Copy link
Member Author

VicDeo commented Dec 2, 2019

@micbar dev is done here.
@mmattel comment above is absolutely irrelevant.

@mmattel
Copy link
Contributor

mmattel commented Dec 2, 2019

@VicDeo ...irrelevant??
all documents which are linked in docs from the outside are always „latest“ because of rolling releases. so if you want to redo this change on 10.4 ...
Maybe @settermjd can explain better

@VicDeo
Copy link
Member Author

VicDeo commented Dec 2, 2019

@mmattel it needs to be done not here, so irrelevant to the PR content. ;)
As you see there are no hardcoded versions in the changes I did.

@mmattel
Copy link
Contributor

mmattel commented Dec 2, 2019

@VicDeo I am in sorry, you are right.

The corresponding URL is defined in:
/lib/private/legacy/defaults.php, line 72
$this->defaultDocVersion = $version[0] . '.' . $version[1]; // used to generate doc links
A change like $version[0] . '.' . $version[1] —> latest should be made there.

@VicDeo VicDeo changed the title Fix doc links in the admin settings [For 10.4] Fix doc links in the admin settings Dec 3, 2019
@micbar micbar merged commit 24443e5 into master Dec 3, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/36238 branch December 3, 2019 10:22
@micbar
Copy link
Contributor

micbar commented Dec 3, 2019

@mmattel @VicDeo @settermjd
IMO we need a discussion about the branching in the docs repo.
I remember there was a discussion about this. Do we have a ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken Links in setupchecks.js
5 participants