-
Notifications
You must be signed in to change notification settings - Fork 482
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
Use new DataTypes module in angular front end #80
Conversation
Angular XHR doesnt follow redirects from POST without an intial preflight task. To get around this, we dont redirect from configure and instead return a url in the response to follow. (Similar to the way we display a page for import setup and redirect to the auth flow).
Angular XHR doesnt follow redirects from POST without an intial preflight task. To get around this, we dont redirect from configure and instead return a url in the response to follow. (Similar to the way we display a page for import setup and redirect to the auth flow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice integration!
client/proxy.conf.json
Outdated
@@ -1,4 +1,9 @@ | |||
{ | |||
"/configure/*":{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just change the endpoint name to /_/configure where it is used on the client and where it is mapped in the server then we dont' need this block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
client/src/app/backend.service.ts
Outdated
@@ -55,6 +58,14 @@ export class BackendService { | |||
.catch(err => this.handleError(err)); | |||
} | |||
|
|||
configure(formData: DataTransferRequest) { | |||
let url = '/configure'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use /_/configure ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
client/src/app/backend.service.ts
Outdated
// Redirect to the export authorization flow after configure. | ||
// this should be returned from the configure request and is checked | ||
// upon creation of the DataTransfer object. | ||
window.location.href=res.nextURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just checked and Angular has a location object you can import and use that is a bit more angular-ly
location.go("the url");
https://angular.io/api/common/Location
https://alligator.io/angular/location-service/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looking at this more, I think the location object is meant to control within the app. When I try location.go("https://external/oauth/url"); it prefixes it with the app domain (localhost/https://external/oauth/url). It also doesnt trigger a page reload to the new path (so having something valid like location.go("/next") only changes the address bar)
The JS guide says to use the window.location instead for this: https://docs.angularjs.org/guide/$location
|
||
@JsonCreator | ||
public DataTransfer( | ||
@JsonProperty(value = "source", required = true) String source, | ||
@JsonProperty(value = "destination", required = true) String destination, | ||
@JsonProperty(value = "transferDataType", required = true) String transferDataType, | ||
@JsonProperty(value = "status", required = true) Status status) { | ||
@JsonProperty(value = "status", required = true) Status status, | ||
@JsonProperty(value = "nextURL", required = true) String nextURL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be a property only on certain types of requests or all? My guess was that this might not be required.
Can we add documentation as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - I added the comment on the class member since adding it at the constructor looked awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the DataTransfer entity would be (eventually) POSTed to a URL when the user wanted to transfer data, but before they have supplied credentials. For my own understanding, what does nextURL do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention was that nextURL would tell the front end client which urls to redirect to after handling the response.
There are three main points where the api server needs to inform the front end where to redirect to:
- in ConfigureHandler (which is wrongly named, should be something like CreateJobHandler), after the front end client creates the job, it should redirect to oauth flow for the source service.
- in ImportSetupHandler, the client should be redirected to the oauth flow for the destination service
- in CopySetup, the client should redirect to the CopyHandler which will start the Copy.
I imagine if this is meant to be a sort of status page, it could be used to redirect the user to the correct page to go to. Example: if the user hasn't supplied creds yet, we could use it to redirect to the oauth flows. Or if theyre checking on the status of the copy, we could redirect them back to the in-status page (or a successful page if the copy is complete)?
Does that make sense?
@@ -13,11 +13,12 @@ | |||
public void verifySerializeDeserialize() throws Exception { | |||
ObjectMapper objectMapper = new ObjectMapper(); | |||
|
|||
DataTransfer transfer = new DataTransfer("testSource", "testDestination", "application/json", DataTransfer.Status.INPROCESS); | |||
DataTransfer transfer = new DataTransfer("testSource", "testDestination", "application/json", DataTransfer.Status.INPROCESS,"nexturltofollow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after INPROCESS,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - reformatted the file with intellij autoformatter.
- Rename DataTransfer type to DataTransferResponse - Rename ConfigureHandler to DataTransferHandler - Use interface for DataTransferResponse instead of class on angular frontend. This is so that the response of the POST request isnt bound to a type but rather to the interface.
some changes: - Rename DataTransfer type to DataTransferResponse - Rename ConfigureHandler to DataTransferHandler - Use interface for DataTransferResponse instead of class on angular frontend. This is so that the response of the POST request isnt bound to a type but rather to the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
…tly turn off encryptedFlow in qa (#94) * update qa index.html to reflect new content, turn on encryptedFlow in qa * turn encryptedFlow back off for qa
This switches over to using angulars http client for the submission of the configure form (instead of htmls form submit).
Switching over the angulars http submit means that the redirects are no longer blindly followed by the browser so as a workaround, I modified the type returned by the configure handler to be a DataTransfer and let the app redirect explicitly to it. This is similar to the way we redirect for the import auth flow and for copy setup.
Note: I manually generated the interface files. I'll look to see if there are tools that will create these for us.
#71
Note: This requires a static content deployment and a new image deploy (#67)