Select apps in group edit analysis page T517 #225

OndraZizka · 2017-02-23T01:28:14Z

No description provided.

jsight · 2017-02-23T14:36:23Z

This needs to be deferred, as the data model is likely to change completely. T517 is not in the current sprint, afaik?

klinki

I left you there couple of comments.

klinki · 2017-02-24T08:43:30Z

...ices/src/main/java/org/jboss/windup/web/services/rest/RegisteredApplicationEndpointImpl.java

    {
-        return this.registeredApplicationService.getAllApplications();
+        if (projectId == null)


I don't like that. MigrationProject entity has applications collection so we can load then with project.
Or add new endpoint getProjectApplications. IMHO getAllApplications should always return all applications and remain parameterless. And this endpoint is not even used in the application, I think we use it only in tests.

"MigrationProject entity has applications collection" - well, it has. But jpamodelgen strips relations away.

Regarding new endpoint resource - that was my first idea too. But the way the subpaths were created, it's not possible - / is used and /{*} is used. Only after that I found I can use regex in the URL, so that could be the solution. When we get back to this task.

Yeah, I think putting it in here is ok, but it probably should be something like getApplicationsByProject, instead of overriding "getAllApplications".

The other paths are used, but they could be changed. :)

I don't think you would even need to change the others, actually. Why not just "/by-project-id/{id}"?

Path of least resistance. If I changed other paths, I would have to go through all components and search where the path is used. Since we don't use a single constant and build them by concatenation, this seemed easier.

To clarify, I am for such path, perhaps shorter one, like, .../id-{id}. It's only it was already this way. Maybe we could have a guide on how to do the endpoint URLs.

I'm not sure that I understand your point there. It seems like each endpoint has a single client service and that service does generally use constants.

klinki · 2017-02-24T08:46:31Z

ui/src/main/webapp/src/app/services/migration-project.service.ts

@@ -64,6 +65,9 @@ export class MigrationProjectService extends AbstractService {
    get(id: number) {
        let headers = new Headers();
        let options = new RequestOptions({ headers: headers });
+        if (!isNumber(id)) {
+            throw new Error("Not an app ID: " + id);


Nope, it is not an app id. It should be project id.
But 👍 for adding parameter validation.

klinki · 2017-02-24T08:47:41Z

services/src/main/java/org/jboss/windup/web/services/model/RegisteredApplication.java

@@ -40,6 +42,7 @@
    public static final String REGISTERED_APPLICATION_ID = "registered_application_id";

    @Id
+    @Access(AccessType.PROPERTY) // Allow accessing ID without Lazy-loading the entity.


How does this work? Does it mean you can load collection of RegisteredApplications and if you access only ID, it will create SQL query getting only ID?

If you have a relation to this, the ID is in the source entity table. And the proxy returns that id. So Hibernate doesn't need to load anything, it's just there.

klinki · 2017-02-24T08:50:25Z

...a/org/jboss/windup/web/services/rest/MigrationProjectRegisteredApplicationsEndpointTest.java

        this.dummyProject = this.dataProvider.getMigrationProject();
    }

    @Test
    @RunAsClient
+    public void testRegisterAppFromProject() throws Exception


Does this test do anything we don't have covered already?

Ah, I see it now. It is wrongly named test. It should not test registering applications, but rather getAllApplications endpoint with projectId parameter?

Right, it should be named testRegisteredAppFromProject(). I don't like the name RegisteredApplication, too long and not much meaningful. I'd rather name the other one UserApplication.

klinki · 2017-02-24T08:53:07Z

ui/src/main/webapp/src/app/components/analysis-context-form.component.html

@@ -16,8 +17,8 @@ <h2 *ngIf="!applicationGroup" i18n>Loading...</h2>
    </div>
    <div class="row">
        <div class="form-group">
-            <label class="col-md-2 control-label" for="migrationPath" i18n>Migration Path</label>
-            <div class="col-md-6">
+            <label class="col-md-2 control-label" for="migrationPath" i18n>Migration Scenario</label>


We agreed we will stick to Migration Path

Forgotten bit.

klinki · 2017-02-24T08:54:12Z

ui/src/main/webapp/src/app/components/analysis-context-form.component.html

-                <button [disabled]="!analysisContextForm.form.valid" class="btn btn-primary" (click)="save()" i18n>Save Configuration</button>
-                <button (click)="cancel()" type="button" class="btn btn-default" i18n>Cancel</button>
+            <div class="col-lg-10 col-lg-offset-2">
+                <button (click)="saveAndRun()" class="btn btn-primary" [disabled]="!analysisContextForm.form.valid" *ngIf="isInWizard" i18n>Save &amp; Run</button>


What is this change good for? It just reorders things...

Makes the source more readable - attributes are in the same order.

That's questionable because you moved *ngIf part to the end so it will take you more time to figure out when it is visible.
But we don't really have any convention on this, so if you think it is better...

klinki · 2017-02-24T08:56:18Z

ui/src/main/webapp/src/app/components/analysis-context-form.component.ts


-    private _migrationPathsObservable:Observable<MigrationPath[]>;
+    private _migrationPathsObservable: Observable<MigrationPath[]>;


Could you do those formatting changes at least in different commit, if not in different PR?

Maybe, but then I wouldn't probably get to it ever.

Ok. At least try to make it in different commit next time :)

Btw maybe we could define some code formatting rules and run some automatic reformatting on all files? (when changes settle down little bit, so we don't have tons of conflicts)

@klinki - I would love to have a code formatter that we could all consistently use. We all use different IDEs, though, I think, so that might be a little harder. It would probably have to be an external tool.

I'm on IDEA now, what do you use?
Also, +1 to formatter if it doesn't control everything, like the one we have for Java. Sometimes the default is not the best, especially for the condensed syntax of TS.

I am on IDEA as well, but I don't think that everyone is. And, I think it would be a good idea to avoid tying to a single IDE anyway.

klinki · 2017-02-24T08:58:18Z

ui/src/main/webapp/src/app/components/analysis-context-form.component.ts

    saveAndRun() {
        this.action = Action.SaveAndRun;
    }
+    save() {
+        this.action = Action.Save;
+    }
    cancel() {


I don't see any need of this reordering.

OndraZizka · 2017-02-27T02:30:48Z

I suggest we merge this (when ok to), because the data layer change won't change much in this PR - just the service and the checked values mapping.

OndraZizka · 2017-02-28T17:10:41Z

I am not sure how to rebase this, standard rebase fails. Maybe by doing a diff and applying that?

OndraZizka · 2017-02-28T17:15:13Z

I have done basically that - detaching, git reset master, git commit --all

klinki · 2017-03-01T10:09:14Z

Why did standard rebase fail?

OndraZizka · 2017-03-02T18:06:53Z

It was a weird rebase, I ended up with master merged into my branch, and then I couldn't push it to my github repo, kept refusing... so I did what I wrote above, and lost those 2 files for some reason

jsight · 2017-03-02T19:05:44Z

retest

jsight · 2017-03-02T19:37:24Z

I think some of the merges have broken this. Just a few things that I noticed:

The path for getting applications is not being generated correctly. I fixed that locally.
AAA appears before each selection item
The selections don't appear to actually affect the analysis runs
Frequently, I get errors like "Already had POJO for id (java.lang.Long) [com.fasterxml.jackson.annotation.ObjectIdGenerator$IdKey@4fb09ae7]" when trying to save the configuration.

klinki · 2017-03-02T20:36:23Z

I will look at it tomorrow and fix it.

OndraZizka · 2017-03-03T00:57:15Z

I forgot I left it in half-done state, only the UI + storing.

OndraZizka · 2017-03-03T02:24:58Z

"The path for getting applications is not being generated correctly. I fixed that locally." - not sure what that means.
AAA removed
"The selections don't appear to actually affect the analysis runs" - I assumed that setting the apps that belong to the group should be enough. Doesn't the launcher take app list from the DB?
"Already had POJO" - yeah I was solving this for a while but then you said that's not something we should be solving now. I understood it related to this issue. Maybe it was to the task as a whole? :)

klinki · 2017-03-03T12:29:51Z

That path thing was my fault, sorry. I fixed that.

klinki · 2017-03-03T13:26:43Z

retest

klinki · 2017-03-03T13:54:21Z

for some reason it calls method shouldBeChecked every few seconds
it allows to submit form without any application selected
those issues with POJOs make it unusable
it feels we really should merge entities AnalysisContext and ApplicationGroup first, because in cause of one of updates fails, you get into inconsistent state (this actually happens right now, AnalysisContext update will proceed successfully, but ApplicationGroup returns error 500).

It didn't test anything new.

Send only relevant fields to server: - id - title - applications

klinki

Even though I think there are still some minor issues, I'm for merging.

All those major issues should be resolved. I just tested it and now it successfully updates the group and it actually changes the analyzed applications so it seems to be working.

OndraZizka · 2017-03-03T15:09:13Z

IMO the POJO issue should be solved at the server. The server service should be able to accept the real data. It doesn't matter in our current situation, but imagine we have 10 different clients. Each client will have to strip the object before sending to the service.

OndraZizka · 2017-03-03T15:10:55Z

"for some reason it calls method shouldBeChecked every few seconds" - that's not a problem of this PR but of the whole app - it's being re-rendered repeatedly. We need to find out why.

klinki · 2017-03-03T18:22:02Z

@OndraZizka Well, for the first item there is much, much more work to be done, if we want to support different clients in the future. And that POJO issue is IMHO caused by RestEasy and @JsonIdentityInfo annotation. (Which I'm really not a big fan of, since it can cause unpredictable outputs). But that's a whole different story for different PR.

jsight · 2017-03-03T22:03:34Z

It would be nice if we had a client library that could deal with cycles properly. I'm sure some of these must exist.

OndraZizka · 2017-03-03T22:34:56Z

How was the "The selections don't appear to actually affect the analysis runs" solved?

OndraZizka · 2017-03-04T01:07:50Z

@klinki The point was not having multiple clients. The point was that this way of doing REST api creates a burden of extra handling on the client side at each place where it is used.

OndraZizka added help wanted Needs Rebase Work in Progress / for discussion labels Feb 23, 2017

OndraZizka requested review from jsight and klinki February 23, 2017 01:28

OndraZizka force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch from 5c44525 to 4a0b3b5 Compare February 23, 2017 08:16

OndraZizka removed the Needs Rebase label Feb 23, 2017

OndraZizka removed the help wanted label Feb 23, 2017

klinki reviewed Feb 24, 2017

View reviewed changes

klinki added the Needs Rebase label Feb 28, 2017

OndraZizka added Needs Rebase and removed Needs Rebase labels Feb 28, 2017

OndraZizka force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch from 0e59d7c to 657bbc4 Compare February 28, 2017 17:14

OndraZizka removed the Needs Rebase label Feb 28, 2017

OndraZizka added Ready to Review and removed Work in Progress / for discussion labels Mar 2, 2017

OndraZizka force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch 2 times, most recently from 0e59d7c to 1875f31 Compare March 2, 2017 15:47

OndraZizka added Work in Progress / for discussion and removed Ready to Review labels Mar 3, 2017

OndraZizka force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch 2 times, most recently from e3d9fec to c840aad Compare March 3, 2017 01:47

klinki force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch from 8167859 to f522bad Compare March 3, 2017 12:13

klinki force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch from adf3754 to dd1d394 Compare March 3, 2017 14:41

OndraZizka and others added 6 commits March 3, 2017 15:41

Select apps in group edit analysis page T517

9d56b68

Update RegisteredApplicationEndpoint - add get by project method

df4d347

Remove redundant test

783a08b

It didn't test anything new.

Update test to use resteasy proxy

cee941d

Fix ApplicationGroupService update method

7c1d828

Send only relevant fields to server: - id - title - applications

Update error messages in ApplicationGroupService update

2d02e92

klinki force-pushed the selectAppsInGroup-editAnalysisPage-T517 branch from dd1d394 to 2d02e92 Compare March 3, 2017 14:41

klinki approved these changes Mar 3, 2017

View reviewed changes

klinki added Ready to Review and removed Work in Progress / for discussion labels Mar 3, 2017

OndraZizka added the Reviewed/Ready to Merge label Mar 6, 2017

OndraZizka merged commit 0f8dac8 into windup:master Mar 6, 2017


		private _migrationPathsObservable:Observable<MigrationPath[]>;
		private _migrationPathsObservable: Observable<MigrationPath[]>;

Select apps in group edit analysis page T517 #225

Select apps in group edit analysis page T517 #225

Conversation

OndraZizka commented Feb 23, 2017 • edited

jsight commented Feb 23, 2017

klinki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraZizka Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraZizka commented Feb 27, 2017

OndraZizka commented Feb 28, 2017

OndraZizka commented Feb 28, 2017

klinki commented Mar 1, 2017

OndraZizka commented Mar 2, 2017

jsight commented Mar 2, 2017

jsight commented Mar 2, 2017

klinki commented Mar 2, 2017

OndraZizka commented Mar 3, 2017

OndraZizka commented Mar 3, 2017 • edited

klinki commented Mar 3, 2017

klinki commented Mar 3, 2017

klinki commented Mar 3, 2017 • edited

klinki left a comment

Choose a reason for hiding this comment

OndraZizka commented Mar 3, 2017

OndraZizka commented Mar 3, 2017

klinki commented Mar 3, 2017

jsight commented Mar 3, 2017

OndraZizka commented Mar 3, 2017

OndraZizka commented Mar 4, 2017

OndraZizka commented Feb 23, 2017 •

edited

OndraZizka Feb 27, 2017 •

edited

OndraZizka commented Mar 3, 2017 •

edited

klinki commented Mar 3, 2017 •

edited