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

Angular6 upgrade #320

Merged
merged 20 commits into from
Dec 3, 2018
Merged

Angular6 upgrade #320

merged 20 commits into from
Dec 3, 2018

Conversation

LotteHofstede
Copy link
Contributor

@LotteHofstede LotteHofstede commented Oct 31, 2018

Upgraded the code base to Angular6.
Also upgraded RXJS to v6 and Webpack to v4.

Added angular/schematics and ngrx/schematics to easily create new components, reducers, effects, etc. using the command line. New files can be created using the ng generate command.

For generating common Angular files, see: https://github.com/angular/angular-cli/wiki/generate#available-schematics
For generating common ngrx files, see: https://github.com/ngrx/platform/tree/master/docs/schematics#schematics
Note that you'll have to add the project's node_modulesto your PATH variable for this to work.

This PR closes #201, all operators are now pipable, including the RxJS store's select operator. Most of this refactoring was done using this tool.

A lot of tests have been updated because the upgrades broke them. I also rewrote a lot of tests that were not using mock services yet. There are still some tests (mostly in the dynamic form code) that I was not able to fix, because the tests heavily rely on the implementation of the services. This should still be fixed, but I don't have enough understanding of the framework to do this myself.
Please note that I wasn't able to test the dynamic forms yet either, because the endpoint that was used for this purpose (I had a gist from Giuseppe for this) does not seem to exist on 4Science's server any longer.

LotteHofstede and others added 16 commits August 24, 2018 13:26
Conflicts:
	package.json
	src/app/+search-page/search-filters/search-filters.component.ts
	src/app/core/auth/auth.effects.ts
	src/app/core/auth/auth.service.ts
	src/app/core/auth/server-auth.service.ts
	src/app/core/data/comcol-data.service.ts
	src/app/core/data/community-data.service.ts
	src/app/core/data/data.service.ts
	src/app/core/data/item-data.service.ts
	src/app/core/shared/dspace-object.model.ts
	src/app/header/header.component.spec.ts
	src/app/shared/auth-nav-menu/auth-nav-menu.component.ts
	src/app/shared/testing/auth-service-stub.ts
	yarn.lock
Conflicts:
	src/app/core/cache/builders/remote-data-build.service.ts
	src/app/core/metadata/metadata.service.ts
	src/app/core/shared/item.model.ts
	src/app/shared/loading/loading.component.ts
	yarn.lock
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@LotteHofstede : Thanks for this!

I've given this a thorough code review & test. It's mostly looking great, but I found a few changes in the code that I had minor questions on (see below).

@atarix83 will need to be aware that there are a lot of changes to the Dynamic Forms code. While the changes themselves look OK to me, and pass tests, as you noted in your description they cannot be manually tested quite yet. So, I'm just calling this out to @atarix83 to make sure he's aware.

I also did a full rebuild of the project with this PR. Overall, everything looks to be working great! I see no changes in behavior between Angular 6 and what's on master.

So, once my questions below are answered, I'd be a 👍 on merging this. Thanks again!

package.json Outdated Show resolved Hide resolved
src/app/core/config/config.service.ts Show resolved Hide resolved
src/app/core/data/comcol-data.service.ts Outdated Show resolved Hide resolved
src/app/shared/form/form.service.spec.ts Outdated Show resolved Hide resolved
src/app/core/cache/response-cache.models.ts Outdated Show resolved Hide resolved
@atarix83
Copy link
Contributor

@tdonohue @LotteHofstede

I'd review this PR too, so please don't merge before I've done.
Tomorrow I'll be able to do.
Thanks

Copy link
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

I've done a first review and found some issues :

  • when I'm not authenticated and I'm trying to retrieve a form configuration through SubmissionFormsConfigService I get a 401 response from REST server, but this cause a SSR error that block all application. You can test with this simple code :
import { SubmissionFormsConfigService } from '../core/config/submission-forms-config.service';

@Component({
  selector: 'ds-home-page',
  styleUrls: ['./home-page.component.scss'],
  templateUrl: './home-page.component.html'
})
export class HomePageComponent {

  constructor(protected formConfigService: SubmissionFormsConfigService) {

  }

  ngOnInit() {
    this.formConfigService.getConfigByName('traditionalpageone')
      .subscribe((r) => console.log(r));
  }
}
  • I tried to render this form configuration https://dspace7.4science.it/dspace-spring-rest/api/config/submissionforms/traditionalpageone and I've found several issues (I refer to the following screenshot) :
    schermata del 2018-11-20 19-03-42

    • in the filed Authors labels for input placeholders are missing (NameFieldParser)
    • field Date of Issue (DsDatePickerComponent) is not rendered
    • a CSS problem in the field Identifier (DynamicQualdropModel) the two input belonging to input-group are not perfectly adjacent
    • fields Type and Language (DsDynamicScrollableDropdownComponent) are not rendered

    this is how the same configuration form is renderend with PR#279
    schermata del 2018-11-20 19-02-58

  • I've update the gist to allow you to test properly the DynmicForm. You should test all the form configuration defined here https://dspace7.4science.it/dspace-spring-rest/#https://dspace7.4science.it/dspace-spring-rest/api/config/submissionforms and compare results with the ones obtained in master branch

@LotteHofstede
Copy link
Contributor Author

Thanks @atarix83 for you feedback and help with testing this part of the code. I'll try to fix it today 🤞

@artlowel
Copy link
Member

when I'm not authenticated and I'm trying to retrieve a form configuration through SubmissionFormsConfigService I get a 401 response from REST server, but this cause a SSR error that block all application. You can test with this simple code

This issue should be fixed now.

I tried to render this form configuration https://dspace7.4science.it/dspace-spring-rest/api/config/submissionforms/traditionalpageone and I've found several issues (I refer to the following screenshot) :

In the meeting before we accepted PR #264 we discussed the big switch in that PR. You explained that the switch was part of ng-dynamic-forms and needed to be copied in to dspace temporarily to be able to make customizations to the lib, but you'd move those customizations to separate components after the upgrade to angular 6.

In this PR Lotte had to upgrade to ng-dynamic-forms 6 in order to be compatible with angular 6, as a consequence the switch was no longer compatible so she had to remove it. Afterwards she made sure that all tests pertaining to the dynamic forms worked. Reverse-engineering that switch to look for customizations to ng-dynamic-forms that aren't covered by tests, I'd consider out of scope of this PR.

@atarix83
Copy link
Contributor

In this PR Lotte had to upgrade to ng-dynamic-forms 6 in order to be compatible with angular 6, as a consequence the switch was no longer compatible so she had to remove it. Afterwards she made sure that all tests pertaining to the dynamic forms worked. Reverse-engineering that switch to look for customizations to ng-dynamic-forms that aren't covered by tests, I'd consider out of scope of this PR.

I'd like to clarify that after this upgrade ds-form component module doesn't work as it do now, so I think a solution should be found in this PR

@tdonohue
Copy link
Member

tdonohue commented Dec 3, 2018

As discussed in our DSpace 7 meeting on Nov 29, merging this as-is.

As noted in the comments above, there are a few known issues related to (as of yet, unused) submission components in Angular 6. However, we've decided it would be easier/more appropriate to resolve those known issues in the upcoming Submission PR (#279), as that is where those components will be used. Atmire has promised support to @atarix83 to help get that Submission PR ready for Angular 6.

@tdonohue tdonohue merged commit a3b4883 into DSpace:master Dec 3, 2018
@ghost ghost removed the needs review label Dec 3, 2018
@benbosman benbosman deleted the angular6-upgrade branch September 11, 2020 07:50
@tdonohue tdonohue added this to the 7.0preview milestone Jan 26, 2021
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Sep 26, 2022
[DSC-745] Keep HTML and XML as plain text

Approved-by: Giuseppe Digilio
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.

RxJS 5.5: Pipeable Operators
4 participants