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

Remove closed issues referenced in docs #3327

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

alex-fusionauth
Copy link
Contributor

  • 441
  • 171
  • 275
  • 494
  • 77
  • 1319
  • added in 1.45
  • 1699
  • 1939
  • 2237
  • 2736

@alex-fusionauth alex-fusionauth requested review from a team as code owners October 10, 2024 18:10
@mooreds
Copy link
Contributor

mooreds commented Oct 11, 2024

will take a look at this in a week or so, @alex-fusionauth . Sorry for the delay.

Copy link
Contributor

@mooreds mooreds left a comment

Choose a reason for hiding this comment

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

Some changes requested.

As you indicated, most of the complexity is around the patch issue. Let's talk about that sync if needed.

description?: string;
method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel comfortable changing the quote choice in this kind of PR. From looking at a few other components, we seem to use single quotes.

Please either reverse or get @lyleschemmerling or another engineer to bless these changes.

}

// Optionally extract one or more segment.
let type = 'literal';
let atom = '';
let type = "literal";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel comfortable changing the quote choice in this kind of PR. From looking at a few other components, we seem to use single quotes.

Please either reverse or get @lyleschemmerling or another engineer to bless these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just had a chat in the engineering huddle with the team this morning. I do think that we are going to go with double quotes, but I want to get that into eslint and then do a big bang commit that cleans up the code per the rules


for (let i = 0; i < url.length; i++) {
let c = url.charAt(i);
atom += c;

if (c === '{') {
if (c === "{") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel comfortable changing the quote choice in this kind of PR. From looking at a few other components, we seem to use single quotes.

Please either reverse or get @lyleschemmerling or another engineer to bless these changes.

@@ -11,6 +11,6 @@ import APIField from 'src/components/api/APIField.astro';

{props.idp_type === 'Google' && <>
<span class="text-green-600">Since 1.44.0</span> <br/>
<strong>If you are using a version of FusionAuth older than 1.44.0</strong>, <code>UsePopup</code> won't work. <code>UseRedirect</code> will continue to work after this date. Please see <a href="https://github.com/FusionAuth/fusionauth-issues/issues/1939">Issue #1939</a> for more. This <a href="/community/forum/topic/2329/upcoming-google-identity-provider-changes">forum post</a> has more details on an available workaround and upgrade process.
<strong>If you are using a version of FusionAuth older than 1.44.0</strong>, <code>UsePopup</code> won't work. <code>UseRedirect</code> will continue to work after this date. Please see [release 1.44.0](/docs/release-notes/#version-1-44-0) for more information. This <a href="/community/forum/topic/2329/upcoming-google-identity-provider-changes">forum post</a> has more details on an available workaround and upgrade process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<strong>If you are using a version of FusionAuth older than 1.44.0</strong>, <code>UsePopup</code> won't work. <code>UseRedirect</code> will continue to work after this date. Please see [release 1.44.0](/docs/release-notes/#version-1-44-0) for more information. This <a href="/community/forum/topic/2329/upcoming-google-identity-provider-changes">forum post</a> has more details on an available workaround and upgrade process.
<strong>If you are using a version of FusionAuth older than 1.44.0</strong>, <code>UsePopup</code> won't work. <code>UseRedirect</code> will continue to work after this date. Please see [the 1.44.0 release notes](/docs/release-notes/#version-1-44-0) for more information. This [forum post](/community/forum/topic/2329/upcoming-google-identity-provider-changes) has more details on an available workaround and upgrade process.

@@ -190,5 +190,5 @@ If you wish to enable an [invisible reCAPTCHA](https://developers.google.com/rec
```

<Aside type="note">
On versions of FusionAuth prior to 1.46.0 you will need to update the JavaScript in order to properly handle the form submit for invisible reCAPTCHA. See [this GitHub issue](https://github.com/FusionAuth/fusionauth-issues/issues/2237) for more information.
On versions of FusionAuth prior to 1.46.0 you will need to update the JavaScript in order to properly handle the form submit for invisible reCAPTCHA. See [release 1.46.0](/docs/release-notes/#version-1-46-0) for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
On versions of FusionAuth prior to 1.46.0 you will need to update the JavaScript in order to properly handle the form submit for invisible reCAPTCHA. See [release 1.46.0](/docs/release-notes/#version-1-46-0) for more information.
On versions of FusionAuth prior to 1.46.0 you will need to update the JavaScript in order to properly handle the form submit for invisible reCAPTCHA. See [the 1.46.0 release notes](/docs/release-notes/#version-1-46-0) for more information.

@@ -501,7 +501,7 @@ FA_HOST=...
curl -H "Authorization: $API_KEY" $FA_HOST'/api/user/search?queryString=data.migrated%3Atrue%0A'
```

This will return all the users who've been migrated as well as a count, subject to the limits of FusionAuth's Elasticsearch integration. See this [GitHub issue](https://github.com/FusionAuth/fusionauth-issues/issues/494) for more on those limits. Here's an example of the output:
This will return all the users who've been migrated as well as a count, subject to the limits of FusionAuth's Elasticsearch integration. See [Maximum Users Returned Workarounds](/docs/get-started/core-concepts/limitations#maximum-users-returned-workarounds) for how to get more from search. Here's an example of the output:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This will return all the users who've been migrated as well as a count, subject to the limits of FusionAuth's Elasticsearch integration. See [Maximum Users Returned Workarounds](/docs/get-started/core-concepts/limitations#maximum-users-returned-workarounds) for how to get more from search. Here's an example of the output:
This will return all the users who've been migrated as well as a count, subject to the limits of FusionAuth's Elasticsearch integration. Prior to version 1.48.0, there were limits on the number of users returned [which required workarounds](/docs/get-started/core-concepts/limitations#maximum-users-returned-workarounds). After version 1.48.0, [use pagination to retrieve all users](/docs/lifecycle/manage-users/search/search#extended-pagination).
Here's an example of the output:

@@ -376,5 +376,5 @@ Navigate to <Breadcrumb>Applications -> Your Application -> OAuth</Breadcrumb> a

* You can view the [example application's codebase](https://github.com/fusionauth/fusionauth-example-node-sso).
* The [Tenant API](/docs/apis/tenants) can be used to manage single sign-on related configuration.
* This guide uses the hosted login pages. If you are using the Login API and building your own pages, [check out the comments on this issue](https://github.com/FusionAuth/fusionauth-issues/issues/171) for guidance.
* This guide uses the hosted login pages. If you are using the Login API and building your own pages, [see our Login API Overview](/docs/lifecycle/authenticate-users/login-api/) for guidance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. If you look at the comments on 171, particularly FusionAuth/fusionauth-issues#171 (comment) then it is clear our overview is not a substitute.

But I don't think we want to document using the login API to recreate SSO at this time (which would require sifting through those comments, making an example app, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This guide uses the hosted login pages. If you are using the Login API and building your own pages, [see our Login API Overview](/docs/lifecycle/authenticate-users/login-api/) for guidance.
* This guide uses the hosted login pages.

So I'd suggest this.

@@ -15,7 +15,7 @@ import SessionsExpiration from 'src/content/docs/lifecycle/authenticate-users/_s
import SSOLogin from 'src/diagrams/docs/lifecycle/authenticate-users/sso-login.astro';
import SSOLogout from 'src/diagrams/docs/lifecycle/authenticate-users/sso-logout.astro';
import { YouTube } from '@astro-community/astro-embed-youtube';
import {RemoteCode} from '@fusionauth/astro-components';
import { RemoteCode } from '@fusionauth/astro-components';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { RemoteCode } from '@fusionauth/astro-components';
import {RemoteCode} from '@fusionauth/astro-components';

Consistency :)

@@ -214,7 +214,7 @@ The **First-party service authorization** mode is the inverse of the **Third-par

With this mode, your OAuth server might display a "permission grant screen" to the user asking if they want to grant the third-party application permissions to your APIs. This isn't strictly necessary and depends on your requirements, but if it is, you want custom scopes.

Custom scopes are not currently supported in FusionAuth; here's the [GitHub tracking issue](https://github.com/FusionAuth/fusionauth-issues/issues/275).
Custom scopes are now supported in FusionAuth; you can find more information within [OAuth Scopes](/docs/get-started/core-concepts/scopes).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Custom scopes are now supported in FusionAuth; you can find more information within [OAuth Scopes](/docs/get-started/core-concepts/scopes).
Custom scopes are now supported in FusionAuth; you can find more information in the [OAuth Scopes documentation](/docs/get-started/core-concepts/scopes).

The reason you need to retrieve the user and modify the data, then use `PUT` to update it, is because of how `PATCH` handles arrays.
[Read this tracking issue for more info.](https://github.com/FusionAuth/fusionauth-issues/issues/441)
{/* TODO: check if this still makes sense */}
<StaticPatchNote />
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not put the static patch note here. I do think that you can remove Read this tracking issue for more info.](https://github.com/FusionAuth/fusionauth-issues/issues/441) and point folks at https://fusionauth.io/docs/apis/#the-patch-http-method

'client-credentials': '/docs/apis/authentication#client-credentials',
'local-bypass': '/docs/apis/authentication#localhost-authentication-bypass'
}
"api-key": "/docs/apis/authentication#api-key-authentication",
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you running Prettier or something similar, or were just going off of rules in VS Code?

<APIAuthenticationIcon type={auth}/>
</a>
{
(authentication || title) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the formatting changes are pretty distracting and it is hard to tell what is content and what is syntax

*/}

<Aside type="note" title="Please read">
<p>When using the PATCH method, you can either use the same request body documentation that is provided for the PUT request for backward compatibility. Or you may use either <a href="https://www.rfc-editor.org/rfc/rfc6902">JSON Patch/RFC 6902]</a> or <a href="https://www.rfc-editor.org/rfc/rfc7396">JSON Merge Patch/RFC 7396</a>. See the <a href="/docs/apis/#the-patch-http-method">PATCH documentation</a> for more information.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing I have a linter check WIP for

An alternative that allows you to use typed objects immediately is to perform a retrieve operation, modify the object in memory, and then execute an update operation. These are functionally equivalent to a single `PATCH` operation.

{/*
TODO: Does this make sense to update fully to allowing PATCH in this way?
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, looking at https://fusionauth.io/docs/sdks/java#patch-requests there is a lot of extra information in there that vanishes when using this aside

showPatch && method == "PUT" && (
<div class="mb-14">
<Astro.self method="PATCH" uri={uri} />
<StaticPatchNote />
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually looks right for the api doc, but is not what we want on the sdk pages

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