Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Add appropriate message if empty bundle #119

Merged
merged 5 commits into from Aug 24, 2020

Conversation

sladyn98
Copy link
Contributor

@sladyn98 sladyn98 commented Aug 9, 2020

This PR fixes the app crashing if the bundle in the local cache is empty and instead adds an appropriate message.

@sladyn98 sladyn98 added the frontend Related to the front-end of the service label Aug 9, 2020
@sladyn98 sladyn98 requested a review from a team as a code owner August 9, 2020 08:02
@sladyn98 sladyn98 linked an issue Aug 9, 2020 that may be closed by this pull request
Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

Looks good but there might be an issue. See the comments.

frontend/src/components/PackageGeneration/Editor.js Outdated Show resolved Hide resolved
Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

So I am not sure how to test the changes in this PR. Can you explain the steps to manually test this?


if (!localStorage.getItem("packageConfigJSON")["bundle"] === undefined ) {
if (localStorage.getItem("packageConfigJSON")["bundle"]["title"] === undefined ) {
this.setState({title: "No title Specified"})
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you expect the user to do under this condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user cannot do anything. He cannot manually change the title unless he goes back and creates a new configuration

@sladyn98
Copy link
Contributor Author

To test manually you need to have npm and mavnen installed
To start Front-end:

cd frontend/
npm start

To start backend

mvn spring-boot:run

Copy link
Contributor

@kwhetstone kwhetstone left a comment

Choose a reason for hiding this comment

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

I think there needs to be some extra logging or at least messaging for the else case.

I think you can add a whole separate ticket for testing.

else {
this.setState({description: JSON.parse(localStorage.getItem("packageConfigJSON"))["bundle"]["description"]})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also notes if there are issues if "bundle" isn't in the packageConfigJSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add another condition to check if the bundle is present ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only if there's not a check earlier for if JSON.parse(localStorage.getItem("packageConfigJSON"))["bundle"] is missing. If it's missing, then you can't do generation, but you should put something out to the console so you can trace where a problem happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are just for the description for the cards so we do not really care if it present or not. If the generation errors out the backend comes back with the 404.

@martinda
Copy link
Contributor

I am having difficulties running this PR. I get these errors in the frontend:

Proxy error: Could not proxy request /favicon.ico from localhost:3000 to http://app-server:8080.
Proxy error: Could not proxy request /api/plugin/getPluginList from localhost:3000 to http://app-server:8080.

@sladyn98
Copy link
Contributor Author

@kwhetstone @martinda Should we add more checks to this PR or is it good to go as is ?

@martinda
Copy link
Contributor

Hi @sladyn98 . I still get the errors I mentioned above. Those need to be addressed. This PR is behind master, so I suggest you start by rebasing it on master. Then write down the steps to test all aspects of this fix, then write a unit test.

@sladyn98
Copy link
Contributor Author

Steps to test this PR:
a) Select plugins and generate a package.
b) Verify if the selected package name appears on the package generation page.
c) If no package is generated or the editor is empty verify the above messages appear
1) Title: No title specified
2) Description: No Description specified

@sladyn98
Copy link
Contributor Author

Then write down the steps to test all aspects of this fix, then write a unit test.

For the testing of react we would need to choose a tool and then setup the architecture for it
https://reactjs.org/docs/testing.html#:~:text=There%20are%20a%20few%20ways,to%2Dend%E2%80%9D%20tests).

@sladyn98
Copy link
Contributor Author

Tested this with three different cases
a) Generating a package
b) Putting my own package
c) Blank page
And it works just fine

@martinda
Copy link
Contributor

I get the infinite spinning wheel :-(

How I run it:

git fetch upstream refs/pull/119/head:refs/remotes/upstream/pull/119
git checkout pull/119
mvn clean
mvn spring-boot:run

In another terminal:

cd frontend
npm install && npm start

The errors:


Proxy error: Could not proxy request /favicon.ico from localhost:3000 to http://app-server:8080.
See https://nodejs.org/api/errors.html#errors_common_system_errors for more information (ENOTFOUND).

Proxy error: Could not proxy request /api/plugin/getPluginList from localhost:3000 to http://app-server:8080.
See https://nodejs.org/api/errors.html#errors_common_system_errors for more information (ENOTFOUND).

Proxy error: Could not proxy request /favicon.ico from localhost:3000 to http://app-server:8080.
See https://nodejs.org/api/errors.html#errors_common_system_errors for more information (ENOTFOUND).

@sladyn98
Copy link
Contributor Author

@martinda you can only run this pr using docker since multiple environments is not yet supported.we need to merge that first to get this working using npm and spring Boot

@martinda
Copy link
Contributor

Ok, so with docker "it runs" but I am still clueless with regards to testing this. What sequence of buttons should I click in the app? Should I expect error messages in the browser?

@sladyn98
Copy link
Contributor Author

sladyn98 commented Aug 23, 2020

Steps to test this PR:
a) Select plugins and generate a package.
b) Verify if the package name appears on the package generation page.
c) If no package is generated or the editor is empty verify the above messages appear

  1. Title: No title specified
  2. Description: No Description specified

@martinda instructions are the same as the previous comment made a few days back. Should I add something more in the instructions?

@sladyn98 sladyn98 merged commit 48558d2 into jenkinsci:master Aug 24, 2020
@sladyn98 sladyn98 added the bugfix A PR that fixes a bug - used by Release Drafter label Aug 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix A PR that fixes a bug - used by Release Drafter frontend Related to the front-end of the service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property "bundle" of null
3 participants