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

Catanie refactor to remove the duplicate pages for anonymous users #823

Merged

Conversation

marcoleorato
Copy link
Contributor

@marcoleorato marcoleorato commented Aug 27, 2021

Description

The aim of the PR is to remove all the duplicate functions created specifically for an anonymous users and adapt the already existing one to account for both an anonymous user or a logged in user.

Motivation

Several part of the catanie code have been duplicated making the site more difficult to maintain as many changes needed to be applied in 2 different pages. There were also some situation in which this was not done leaving, very small, issue that only appeared if you were logged out.
Also the "double" site allow for situations in which a user is logged in but does not realize it since he accessed the anonymous part of the website without knowing.
This also solve issue #807 better than just changing the routing.

Fixes & Changes:

The flag "searchPublicDataEnabled" is now unused as it's now needed to see public datasets while logged in.
Removed all the anonymous pages and functions.
Modified the layout, header, datasets and datasets details to show the correct elements if an user is not logged in.

Full change list

Removed references to anonymous pages that have been removed in the refactor.

CI/ESS/styles.scss
CI/MAXIV/maxiv-theme.scss
CI/RFI/theme.scss
src/styles.scss

Changed the expected url before login from /anonymous/datasets to /datasets as the anonymous page was removed.

cypress/integration/users-login.spec.js

Removed the anonymous routes, changed the routing a bit to redirect to /datasets instead of /anonymous/datasets.

src/app/app-routing/app-routing.module.ts
src/app/app-routing/datasets.guard.ts

Added variable to hide part of the UI not visible to anonymous users. Made a few small UI changes to have it work slightly better. Added a reload where needed (mostly visible columns and datasets) when a user login or logout. Added a class "anonymous-site" just below app-app-layout (selector app-app-layout .anonymous-site) that allow for adding extra styles only visible to anonymous user if needed.

src/app/_layout/app-header/app-header.component.html
src/app/_layout/app-header/app-header.component.scss
src/app/_layout/app-header/app-header.component.ts
src/app/_layout/app-layout/app-layout.component.html
src/app/_layout/app-layout/app-layout.component.scss
src/app/_layout/app-layout/app-layout.component.ts
src/app/_layout/app-main-layout/app-main-layout.component.html
src/app/_layout/app-main-layout/app-main-layout.component.scss
src/app/_layout/app-main-layout/app-main-layout.component.spec.ts
src/app/_layout/app-main-layout/app-main-layout.component.ts
src/app/_layout/layout.module.ts
src/app/shared/modules/breadcrumb/breadcrumb.component.scss
src/app/users/login/login.component.html
src/app/users/login/login.component.scss
src/app/users/login/login.component.ts
src/app/datasets/dashboard/dashboard.component.html
src/app/datasets/dashboard/dashboard.component.scss
src/app/datasets/dashboard/dashboard.component.ts
src/app/datasets/dataset-detail/dataset-detail.component.html
src/app/datasets/dataset-detail/dataset-detail.component.scss
src/app/datasets/dataset-detail/dataset-detail.component.ts
src/app/datasets/dataset-details-dashboard/dataset-details-dashboard.component.html
src/app/datasets/dataset-details-dashboard/dataset-details-dashboard.component.ts
src/app/datasets/dataset-table/dataset-table.component.scss
src/app/datasets/datasets-filter/datasets-filter.component.scss

Removed searchPublicDataEnabled flag as the buttons are now needed to see public data without having to log out.

src/app/datasets/dataset-table-actions/dataset-table-actions.component.html
src/app/datasets/dataset-table-actions/dataset-table-actions.component.ts

Changed the default value of isPublished in the filters as "" as when lookin at your datasets you see both public and non public datasets you own.

src/app/state-management/actions/datasets.actions.ts
src/app/state-management/models/index.ts
src/app/state-management/state/datasets.store.ts

Removed anonymousquery since it's no longer used.

src/app/shared/sdk/services/custom/Dataset.ts
src/app/shared/sdk/services/custom/DerivedDataset.ts
src/app/shared/sdk/services/custom/RawDataset.ts

Removed the getAnonymousFilters and readded the isPublished flag to the query. This is now an empty string by default, >in which case it's not sent to the backend as not needed.

src/app/state-management/selectors/datasets.selectors.spec.ts
src/app/state-management/selectors/datasets.selectors.ts

Files removed as no longer needed.

src/app/_layout/anonymous-layout/anonymous-layout.component.html
src/app/_layout/anonymous-layout/anonymous-layout.component.scss
src/app/_layout/anonymous-layout/anonymous-layout.component.spec.ts
src/app/_layout/anonymous-layout/anonymous-layout.component.ts
src/app/_layout/login-header/_login-header-theme.scss
src/app/_layout/login-header/login-header.component.html
src/app/_layout/login-header/login-header.component.scss
src/app/_layout/login-header/login-header.component.spec.ts
src/app/_layout/login-header/login-header.component.ts
src/app/_layout/login-layout/login-layout.component.html
src/app/_layout/login-layout/login-layout.component.scss
src/app/_layout/login-layout/login-layout.component.spec.ts
src/app/_layout/login-layout/login-layout.component.ts
src/app/datasets/anonymous-dashboard/anonymous-dashboard.component.html
src/app/datasets/anonymous-dashboard/anonymous-dashboard.component.scss
src/app/datasets/anonymous-dashboard/anonymous-dashboard.component.spec.ts
src/app/datasets/anonymous-dashboard/anonymous-dashboard.component.ts
src/app/datasets/anonymous-details-dashboard/_anonymous-details-dashboard-theme.scss
src/app/datasets/anonymous-details-dashboard/anonymous-details-dashboard.component.html
src/app/datasets/anonymous-details-dashboard/anonymous-details-dashboard.component.scss
src/app/datasets/anonymous-details-dashboard/anonymous-details-dashboard.component.spec.ts
src/app/datasets/anonymous-details-dashboard/anonymous-details-dashboard.component.ts
src/app/datasets/anonymous-details/_anonymous-details-theme.scss
src/app/datasets/anonymous-details/anonymous-details.component.html
src/app/datasets/anonymous-details/anonymous-details.component.scss
src/app/datasets/anonymous-details/anonymous-details.component.spec.ts
src/app/datasets/anonymous-details/anonymous-details.component.ts

Tests included/Docs Updated?

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)
  • Docs updated?
  • New packages used/requires npm install?
  • Toggle added for new features?
  • Requires update of catamel API?

Modified the header to be the same for logged in and anonymous pages.
Changed a bit the style of the header to be fixed and not change based on the vh and change the main div to follow resize correctly.
Enable again the returnUrl in the login to redirect to the returnUrl instead of always datasets/.
If you are already logged in it will figure as such in the header. Also if you open the login page while logged in you get redirected to datasets.
Updated all the styles removing the reference to the login-header.
Changed the logoutNavigate test as the anonymous site is accessed going to "/anonymous/" on logout and not "".
Removed all the "anonymous" only page and functionality and change the default pages to show functionalities based on the login status.
Removed the use of the searchPublicDataEnabled flag as it become needed to see public data while logged in.
Updated some minor functions to work better with the changes.
@nitrosx
Copy link
Contributor

nitrosx commented Aug 27, 2021

@marcoleorato thank you so much for taking care of this.
I will check it Monday morning and provide feedback.
If you have time, could you please check why E2E tests fail and increase the coverage?

@marcoleorato
Copy link
Contributor Author

@nitrosx Friday was a bit late so I'm going to fix it today.
For the e2e tests I already found the possible issue (it should be cypress-io/cypress#2640). The problem is within datasets-public.spec.js at line 30-32, the way the "get" and "contains" is written is not reliable (it does succeed with "npx cypress open" and fail around 50% of times with "npx cypress run") I already fixed it locally removing the "get" and it seems to work. In other test I saw a similar problem that was fixed adding a wait(5000) to be sure the request finished before the get is executed.

I'm waiting to push the changes as I'm also going to add a few unit test to fix the coverage.

@nitrosx
Copy link
Contributor

nitrosx commented Aug 31, 2021

@marcoleorato what is the best way to test all the changes that you implemented?
Thanks

@marcoleorato
Copy link
Contributor Author

The best way to test would probably to compare the pages with the current develop branch. While logged in with the page at the same route and logged out with the page at the /anonymous/* url.
The changes affect the header, dashboard, dataset details view and the action that happen when you log in/out. But the only "visible" change should be the presence of two additional buttons on the dashboard with a "My Data" and "All Public Data" buttons and not having /anonymous/ in the url. Everything else should be/work in the same way as before.

There are also a few changes to the style that are different from develop, so maybe it could be useful to also have Henrik as reviewer since he was working on the style in the last months.

@nitrosx
Copy link
Contributor

nitrosx commented Sep 6, 2021

@marcoleorato: I agree with you. I will ask @henrikjohansson712 to take a look and I will make sure to set aside sometime to test this week.

@stephan271
Copy link
Member

@marcoleorato could you have a look at the merge conflicts ?

After fixing the merge conflicts: is anything preventing us from merging ? I think this change should enter as soon as possible. Any objections from the colleagues ?

@marcoleorato
Copy link
Contributor Author

@stephan271 I have updated the PR branch with the latest changes from develop, now it should be possible to merge without issues if everything else is OK

Copy link
Contributor

@nguyenlinhlinh nguyenlinhlinh left a comment

Choose a reason for hiding this comment

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

Hi @marcoleorato, Thank for the work.

@marcoleorato
Copy link
Contributor Author

A small update regarding this, I am going to make a PR for the backend to remove the anonymousquery from that too.
Also I found a small bug in how the current user is found so I need to add a small fix to the PR before it's ready to be merged. In short what happen is that if you reload the page the user is found only on the login, datasets and settings page. On the other it looks like you are not logged in. I'll be looking into it as soon as I have some time.

@stephan271
Copy link
Member

Thanks for taking over the catamel side of it !

@marcoleorato marcoleorato marked this pull request as draft September 24, 2021 13:46
@marcoleorato marcoleorato marked this pull request as ready for review September 30, 2021 14:54
@marcoleorato
Copy link
Contributor Author

I fixed the small issue I found for the login, also added the change for the issue #836 adding ingestor to the list of functional accounts.

There is also, now, another PR in catamel for the backend part for the removal of anonymousquery.

@henrikjohansson712 henrikjohansson712 merged commit 2151568 into SciCatProject:develop Oct 26, 2021
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.

5 participants