Fixed: App can only be accessed on localhost #1697

aarishshahmohsin · 2024-03-10T07:12:26Z

What kind of change does this PR introduce?

This PR fixes the problem of the portal not being able to be accessed if not on localhost.

Issue Number:

Fixes #1244

Did you add tests for your changes?

Yes

Snapshots/Videos:

If relevant, did you update the documentation?

Yes

Summary

This issue was caused mainly by the case in which the API url contains 'localhost'.

Does this PR introduce a breaking change?

No

Other information
2 problems occurred while working on the issue.

One problem that needs to be fixed is the plugin store, which is coded incorrectly. in the src/components/AddOn/support/services/Plugin.helper.ts file.

    const result = await fetch(`http://localhost:${process.env.PORT}/store`);
    const result = await fetch(`http://localhost:3005/installed`);

are both fetching endpoints that do not exist and hence the addon store component also does not work.

The navbar button is not working on mobile devices.

Should I make issues for them?

Have you read the contributing guide?

Yes

github-actions · 2024-03-10T07:12:50Z

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

The overall code coverage drops below the target threshold of the repository
Any file in the pull request has code coverage levels below the repository threshold
Merge conflicts

The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.

Reviewers

Do not assign reviewers. Our Queue Monitors will review your PR and assign them.
When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

comments in this PR or
our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

arbitrators of future discussions with other contributors about the validity of your changes
point of contact for evaluating the validity of your work
person who verifies matching issues by others that should be closed.
person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

PRs with issues not assigned to you will be closed by the reviewer
Fix the first comment in the PR so that each issue listed automatically closes

Other

🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
Read the CONTRIBUTING.md file make

palisadoes · 2024-03-10T07:54:37Z

setup.ts

+      message: 'Would you like to set up a remote host?',
+      default: true,
+    });
+    shouldSetRemoteHost = shouldSetRemoteHostResponse;


This is unnecessary, setup already asks for the REACT_APP_TALAWA_URL value that includes a TCP port. BTW, You cannot assume the port will never change.

You are duplicating the prompting for an existing parameter. You should be able to derive the necessary information from the REACT_APP_TALAWA_URL value.

This is not asking for API endpoint, but for solving the problem.
If we are accessing the admin portal from the network and the API endpoint contains "localhost", it causes the error. So asking for the remote URL is to change the URL into a URL that can be accessed without any problem.

But if the Admin URL does not explicitly show the port, then the developer would manually have to change the api url.

palisadoes · 2024-03-10T07:56:16Z

src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.test.tsx

-
-    render(
-      <ApolloProvider client={client}>
-        <Provider store={store}>


Why was this removed?

this test was redundant and was causing problems while testing. Removing it did not have an impact on the branch and line coverage.

palisadoes · 2024-03-10T07:56:53Z

src/setup/askForRemoteUrl/askForRemoteUrl.test.ts

+      {
+        type: 'input',
+        name: 'url',
+        message: `Enter your remote url in the format 'http://hosturl/' (don't enter the port)`,


This not necessary, setup already asks for you to Enter your talawa-api endpoint using the REACT_APP_TALAWA_URL value.

palisadoes · 2024-03-10T07:57:18Z

src/setup/askForRemoteUrl/askForRemoteUrl.ts

+  const ans =
+    input.includes(':') && input.indexOf(':') == input.lastIndexOf(':');
+  const urlRegex = /^(https?|ftp):\/\/[^\s/$.?#].[^\s]*$/;
+  return ans && urlRegex.test(input);


This not necessary, setup already asks for you to Enter your talawa-api endpoint using the REACT_APP_TALAWA_URL value.

aarishshahmohsin · 2024-03-10T08:21:00Z

@palisadoes this is to fix the immediate error, the best practice would have been to give an error if the admin portal is being accessed remotely and the API endpoint contains "localhost"

palisadoes · 2024-03-10T08:39:55Z

So you are saying we are fixing to problems:

Admin not running on localhost
API not running on localhost

This requires the different prompts in setup. This was also why you asked the question about automatically determining the value because you were referring to the non localhost Admin scenario. I didn't understand the context of the question.

We need to solve for both scenarios.

It would be greatly preferred to make the Admin app automatically get its Admin related information. For example some browsers could be addressing Admin on IPv4 and others on IPv6 which would make a configured value for this inappropriate. Some could be addressing the app via network address translation (NAT) which would add more unpredictability.

If that's not possible, which I doubt, then the prompt needs to be clearer. It was not apparent that this was related to the Admin portal.

aarishshahmohsin · 2024-03-10T10:01:03Z

So you are saying we are fixing to problems:
1. Admin not running on localhost

2. API not running on localhost
This requires the different prompts in setup. This was also why you asked the question about automatically determining the value because you were referring to the non localhost Admin scenario. I didn't understand the context of the question.

We need to solve for both scenarios.

It would be greatly preferred to make the Admin app automatically get its Admin related information. For example some browsers could be addressing Admin on IPv4 and others on IPv6 which would make a configured value for this inappropriate. Some could be addressing the app via network address translation (NAT) which would add more unpredictability.

If that's not possible, which I doubt, then the prompt needs to be clearer. It was not apparent that this was related to the Admin portal.

@palisadoes Rethinking, I think the best approach would be to set the REACT_APP_TALAWA_URL to the remote network link instead of this workaround on the admin side. So, if the developer wants to access the portal remotely, they will set the API link for example "http://192.168.1.4:4000/graphql" instead of "http://localhost:4000/graphql", which would fix the error without needing this PR.
This could be included in the documentation and the setup prompt asking the API URL and then this PR's workaround can be scrapped. It would be the cleanest solution.

the remote URL can be printed when the API server is started

palisadoes · 2024-03-10T14:57:36Z

Not necessarily, this PR may still be valid.

Please test this out. The issue was originally created because things broke when:

Admin and API were on different hosts or
Admin was accessed from a remote system, not localhost

aarishshahmohsin · 2024-03-10T16:13:48Z

Not necessarily, this PR may still be valid.

Please test this out. The issue was originally created because things broke when:
1. Admin and API were on different hosts or

This PR did not impact this case

2. Admin was accessed from a remote system, not localhost

This PR is fixing this only case.

palisadoes · 2024-03-10T16:20:50Z

Does this PR still need to be modified to a documentation only update?

aarishshahmohsin · 2024-03-10T16:24:07Z

Does this PR still need to be modified to a documentation only update?

It would avoid having unnecessary additions to the code.

palisadoes · 2024-03-10T16:30:26Z

Please make the documentation modifications the core of this PR

codecov · 2024-03-10T17:32:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.39%. Comparing base (e04abf3) to head (333fcc8).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1697   +/-   ##
========================================
  Coverage    97.39%   97.39%           
========================================
  Files          168      168           
  Lines         4148     4148           
  Branches      1206     1210    +4     
========================================
  Hits          4040     4040           
+ Misses         105      104    -1     
- Partials         3        4    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aarishshahmohsin added 4 commits March 8, 2024 19:51

fixed remote host problem

945b2b0

made doc changes

49d315b

fixed test

c86cad4

increased coverage

55e4a69

aarishshahmohsin mentioned this pull request Mar 10, 2024

Fixed: App can only be accessed on localhost #1687

Closed

palisadoes requested changes Mar 10, 2024

View reviewed changes

Cioppolo14 added the Ready 4 PR Review label Mar 10, 2024

Cioppolo14 requested review from Kevoniat and beingnoble03 March 10, 2024 13:19

changed to doc based solution

333fcc8

palisadoes approved these changes Mar 11, 2024

View reviewed changes

palisadoes merged commit 093fda6 into PalisadoesFoundation:develop Mar 11, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed: App can only be accessed on localhost #1697

Fixed: App can only be accessed on localhost #1697

aarishshahmohsin commented Mar 10, 2024

github-actions bot commented Mar 10, 2024

palisadoes Mar 10, 2024

aarishshahmohsin Mar 10, 2024 •

edited

Loading

palisadoes Mar 10, 2024

aarishshahmohsin Mar 10, 2024

palisadoes Mar 10, 2024

palisadoes Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024 •

edited

Loading

palisadoes commented Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024 •

edited

Loading

palisadoes commented Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024

palisadoes commented Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024

palisadoes commented Mar 10, 2024

codecov bot commented Mar 10, 2024

Fixed: App can only be accessed on localhost #1697

Fixed: App can only be accessed on localhost #1697

Conversation

aarishshahmohsin commented Mar 10, 2024

github-actions bot commented Mar 10, 2024

Our Pull Request Approval Process

Testing Your Code

Reviewers

Reviewing Your Code

CONTRIBUTING.md

Other

palisadoes Mar 10, 2024

Choose a reason for hiding this comment

aarishshahmohsin Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

palisadoes Mar 10, 2024

Choose a reason for hiding this comment

aarishshahmohsin Mar 10, 2024

Choose a reason for hiding this comment

palisadoes Mar 10, 2024

Choose a reason for hiding this comment

palisadoes Mar 10, 2024

Choose a reason for hiding this comment

aarishshahmohsin commented Mar 10, 2024 • edited Loading

palisadoes commented Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024 • edited Loading

palisadoes commented Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024

palisadoes commented Mar 10, 2024

aarishshahmohsin commented Mar 10, 2024

palisadoes commented Mar 10, 2024

codecov bot commented Mar 10, 2024

Codecov Report

aarishshahmohsin Mar 10, 2024 •

edited

Loading

aarishshahmohsin commented Mar 10, 2024 •

edited

Loading

aarishshahmohsin commented Mar 10, 2024 •

edited

Loading