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

Added links to workbench #46

Conversation

SyntaxXeror
Copy link
Contributor

Just a quick PR for feedback on the workbench.

ATTENTION: For now, there is no test.

@SyntaxXeror SyntaxXeror self-assigned this Apr 24, 2023
* DUGK-69 Workbench buttons added (no test)

* DUGK-69 Fixed unit test after workbench remake

* DUGK-69 Changes in correlation to PR review comments

* Mark active menu

* DUGK-69 envUtil is now dynamic

* DUGK-69 Refactored LinkButtons
It now maps icons to links enumorator.
Size is now given as prop, but not working.
envUtil provides interface.

* fixed workbench test

* DUGK-69 Excluded config from codecliamte
Fixed bug in icon mathcing.

* DUGK-69 Added link construction with User1

* add redux to menu state

* formatting

* make LinkButtons generate url from user name

* formatting

* update unit test for SignIn

* format

* DUGK-69 fixed eslint error.
Operation is according to [documentation](https://redux-toolkit.js.org/usage/immer-reducers#linting-state-mutations)

* removed client env from public

* updated .gitignore to exclude client/public/env.js

* DUGK-69 Moved domain logic to envUtil.
Also added example of component documentation

* DUGK-69 Renamed states

* DUGK-69 clean up and documentation

* DUGK-69 Fixed Auth Tests

---------

Co-authored-by: Omar <[email protected]>
Co-authored-by: Burnfarm <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2023

Codecov Report

Merging #46 (865a6cd) into feature/distributed-demo (4aac6f7) will increase coverage by 3.60%.
The diff coverage is 63.15%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                     Coverage Diff                      @@
##           feature/distributed-demo      #46      +/-   ##
============================================================
+ Coverage                     53.35%   56.95%   +3.60%     
============================================================
  Files                            25       34       +9     
  Lines                           283      388     +105     
  Branches                          7       11       +4     
============================================================
+ Hits                            151      221      +70     
- Misses                          126      158      +32     
- Partials                          6        9       +3     
Flag Coverage Δ
client-browser-test ∅ <ø> (∅)
client-unit-test 52.90% <63.15%> (+6.04%) ⬆️
lib-microservice 88.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/src/AppProvider.tsx 0.00% <0.00%> (ø)
client/src/page/Menu.tsx 0.00% <0.00%> (ø)
client/src/page/MenuItems.tsx 0.00% <ø> (ø)
client/src/util/envUtil.ts 0.00% <0.00%> (ø)
client/src/route/auth/Signin.tsx 78.94% <50.00%> (-10.34%) ⬇️
client/src/components/LinkButtons.tsx 100.00% <100.00%> (ø)
client/src/components/LinkIconsLib.tsx 100.00% <100.00%> (ø)
client/src/route/workbench/Workbench.tsx 100.00% <100.00%> (ø)
client/src/store/auth.slice.ts 100.00% <100.00%> (ø)
client/src/store/menu.slice.ts 100.00% <100.00%> (ø)
... and 1 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@prasadtalasila
Copy link
Contributor

prasadtalasila commented Apr 28, 2023

@SyntaxXeror and @Omarley7 , may be I'm doing something very wrong.
When this last commit gets pulled to my computer, I can see the changed files, but no changes on the react site itself. The redux state persistence doesn't show up on the menu. Also the links on the workbench page don't get updated.
The commands I followed were:

yarn install
yarn build
yarn configapp dev
yarn start

@Omarley7
Copy link
Contributor

@prasadtalasila That is very strange. Can you verify the following behavior?

  • Redux currently only keeps the menu bar open, when navigating between routes.
  • While the app is running, change REACT_APP_URL_WORKBENCH: 'https://example.com/',. Run yarn configapp dev . Then refresh the page, enter email and navigate to Workbench. The tooltip should show a new base url.

@prasadtalasila
Copy link
Contributor

@prasadtalasila That is very strange.

  • While the app is running, change REACT_APP_URL_WORKBENCH: 'https://example.com/',. Run yarn configapp dev . Then refresh the page, enter email and navigate to Workbench. The tooltip should show a new base url.

This it does. But the tooltip needs to show baseurl+specific suffix for each icon. Only then can we go to pages of the respective tools.

I see that the username capture functionality seems to be in place. Do you use this username to compose relative urls?

@Omarley7
Copy link
Contributor

The tooltip is showing {workbenchurl}/{username}/{btnEndpoint} correctly on my end 🤔
You're not even getting the username from the sign-in form?

@prasadtalasila
Copy link
Contributor

The tooltip is showing {workbenchurl}/{username}/{btnEndpoint} correctly on my end 🤔 You're not even getting the username from the sign-in form?

My mistake. The tooltip correctly shows the https://ece.au.dk/user1/terminals/main URL. Now I am scratching head about the discrepancy. I must have been looking at the previous commit. Sorry about the confusion.

Is this PR complete with tests? or you are going to commit more before it can be potentially merged?

@Omarley7
Copy link
Contributor

I'm working on the lasts tests now. About commit unittest for the component. I'll move tests of utilities and Redux implementations to next PR

Fixed codeclimate issue in menu.
Included .ts files in test coverage

jest.deepUnmock('components/LinkButtons');

const buttons: KeyLinkPair[] = [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@Omarley7
Copy link
Contributor

Hmm, E2E tests are still failing, I'm taking a look right now.
1 codeclimate duplication issue has been introduced, but I fail to see how to avoid it.

Coverage is now above threshold.
Fixed menu tests.
@Omarley7 Omarley7 self-assigned this Apr 29, 2023
@Omarley7
Copy link
Contributor

@prasadtalasila Should be ready to merge now.
This PR will resolve issue #42 since the test coverage is now above the threshold.
It is also now required to run yarn configapp before using the application, since there's no longer any default env.js left in public.

@prasadtalasila
Copy link
Contributor

The code looks good. A few suggestions:

  1. remove recharts package

  2. rename "email address" field to "username". There is a variable called email in signin.tsx. This and it's state handler need to be renamed as well.

  3. correct string in env.js: REACT_APP_WORKBENCHLINK_VNCDESKTOP: '/tools/vnc/?password=vncpassword', in config/ files.

  4. The yarn configapp #prod | dev has been made mandatory. A comment next to this command in README.md will let the developer know about the mandatory nature of this command.

  5. All the tsx files have import * as React from 'react'; as the first line. Is it possible to import only specific items and get the work done? If yes, this can be a good task for next PR.

  6. The comment to LinkButtons function object is useful. Is the following comment written or generated? Also a correction on one of the comment text:

    /**
     *
     * The link is constructed by appending the user name to the end of the URL for the react site, and then appending the value of the environment variable to the end of that.
     * For example, if the URL for the react site is https://foo.com, the user name is "user1", and the value of the environment variable is "/my-workbench", then the link will be https://foo.com/user1/my-workbench.
    */
  7. The code structure among envUtils, LinkIconsLib and LinkButtons is very good.

  8. There seem to be no tests for src/store code. All the test coverage seems to be incidental from integration tests. If that's the case, I suggest the redux code to next PR. But, if you would like to add tests in this PR, that's also fine.

@Omarley7
Copy link
Contributor

@prasadtalasila

  1. All the tsx files have import * as React from 'react'; as the first line. Is it possible to import only specific items and get the work done? If yes, this can be a good task for next PR.

This is a bit tricky. Normally we would use import React from 'react' but that gives errors with the unit tests. They will start giving errors:
TypeError: Cannot read properties of undefined (reading 'createElement')
If it doesn't give big performance issues, I'd like to keep it as is.

  1. The comment to LinkButtons function object is useful. Is the following comment written or generated? Also a correction on one of the comment text:

This was completely autogenerated. I've cleaned it up a bit and taken your correction into account.

I've added tests for the redux store. We have another branch, where envUtil is tested, but it generates problems for the Auth tests. I'll bring that in the next PR.

@prasadtalasila
Copy link
Contributor

Code Climate has analyzed commit f4be833 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2
View more on Code Climate.

The new .codeclimate.yml is supposed to have removed this kind of warnings. Are any modifications required there?

@codeclimate
Copy link

codeclimate bot commented Apr 30, 2023

Code Climate has analyzed commit 865a6cd and detected 0 issues on this pull request.

View more on Code Climate.

@Omarley7
Copy link
Contributor

The new .codeclimate.yml is supposed to have removed this kind of warnings. Are any modifications required there?

The new configuration raised the mass from unknown to the correct default value of 45 for typescript, which was sufficient for all previous cases.

I've just raised it to 55 for typescript.

@prasadtalasila prasadtalasila merged commit af17f91 into INTO-CPS-Association:feature/distributed-demo Apr 30, 2023
Artin13 pushed a commit to Artin13/DTaaS that referenced this pull request May 19, 2023
  - Updates the signin page to require username input. The
    user can't get into the site without a username.
  - Uses  redux state management to keep track of
    logged in username and highlight the active menu option.
    This username is used to construct the workbench links.
  - Updates codeclimate.yml to increase duplication threshold.

---------

Co-authored-by: Asger <[email protected]>
Co-authored-by: Burnfarm <[email protected]>
Co-authored-by: Omar <[email protected]>
Artin13 pushed a commit to Artin13/DTaaS that referenced this pull request May 19, 2023
  - Updates the signin page to require username input. The
    user can't get into the site without a username.
  - Uses  redux state management to keep track of
    logged in username and highlight the active menu option.
    This username is used to construct the workbench links.
  - Updates codeclimate.yml to increase duplication threshold.

---------

Co-authored-by: Asger <[email protected]>
Co-authored-by: Burnfarm <[email protected]>
Co-authored-by: Omar <[email protected]>
Artin13 pushed a commit to Artin13/DTaaS that referenced this pull request May 25, 2023
  - Updates the signin page to require username input. The
    user can't get into the site without a username.
  - Uses  redux state management to keep track of
    logged in username and highlight the active menu option.
    This username is used to construct the workbench links.
  - Updates codeclimate.yml to increase duplication threshold.

---------

Co-authored-by: Asger <[email protected]>
Co-authored-by: Burnfarm <[email protected]>
Co-authored-by: Omar <[email protected]>
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.

4 participants