Addressing Continuous Refreshing in Development Environment

[chandel-aman]

What kind of change does this PR introduce?

Bug

Issue Number:

Fixes #1370

Did you add tests for your changes?

Yes

Snapshots/Videos:

1370.mp4

Summary

The issue of continuous refreshing occurs when local storage data is overwritten by another application running on the same port as Talawa-admin. This PR addresses the problem by introducing a prefix added to the key stored in local storage, ensuring the uniqueness of the key and preventing it from being overwritten.

In the first part of the video, we run the current Talawa-admin on port 3000 to log in. Without logging out, we stop the Talawa-admin server and run another application on port 3000. We observe token conflicts, which modify the value of the token. When we stop this project and start Talawa-admin again, we encounter the issue of continuous refreshing.

In the second part of the video, we run the updated Talawa-admin on port 3000. After logging in, we notice that the key stored in local storage has been prefixed. When we run another project on port 3000, we observe the addition of some data, but there are no conflicts. Upon starting Talawa-admin again, we successfully log in without facing any refreshing issues.

Other information

While this issue is not likely to occur in production, it significantly hinders the development process when contributors are working on multiple projects.

Have you read the contributing guide?

Yes

[github-actions]

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:

1. The overall code coverage drops below the target threshold of the repository
2. Any file in the pull request has code coverage levels below the repository threshold
3. 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:

1. comments in this PR or
2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

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

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

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

Other

1. 🎯 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.
2. Read the CONTRIBUTING.md file make

[chandel-aman]

@palisadoes Could you please review this PR?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (655b6b3) 92.77% compared to head (97a858c) 97.32%.
Report is 169 commits behind head on develop.

❗ Current head 97a858c differs from pull request most recent head 290f9c9. Consider uploading reports for the commit 290f9c9 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1466      +/-   ##
===========================================
+ Coverage    92.77%   97.32%   +4.55%     
===========================================
  Files          134      134              
  Lines         3238     3443     +205     
  Branches       904     1040     +136     
===========================================
+ Hits          3004     3351     +347     
+ Misses         225       87     -138     
+ Partials         9        5       -4     

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

[Cioppolo14]

@chandel-aman Please fix the failing test, or document here why the failed test is acceptable. Thank you.

[palisadoes]

@beingnoble03 @rishav-jha-mech Please take a look. This is a show stopper bug

[chandel-aman]

@chandel-aman Please fix the failing test, or document here why the failed test is acceptable. Thank you.

The check for the PR workflow on the schema.graphl file is showing some error, which is not related to any changes made in this PR.

Regarding the tests, none of them are failing, and they have 100% coverage.

[palisadoes]

Please fix the merge conflicts.

We have a bug in the git commit githooks workflow which keeps generating files in talawa-admin-docs. Please remove them from this PR while we figure out a fix.

[chandel-aman]

Certainly! I'll work on fixing the merge conflicts.

[beingnoble03]

@chandel-aman can you please explain the approach you have taken here? And, how does this fix the issue?

[chandel-aman]

@chandel-aman can you please explain the approach you have taken here? And, how does this fix the issue?

Sure!

Issue Explanation:

• The problem arises due to conflicts in data being stored by multiple projects on the same port in local storage.

Example Scenario:

• Running Talawa-admin on port 3000, logging in, and stopping the server without logging out causes data stored in local storage to persist.
• Running a different project (Project A) on port 3000, which also stores data in local storage, results in the Talawa-admin token being overridden by the Project A token.

Resulting Problem:

• When restarting the Talawa-admin server, it retrieves all data from local storage. However, some data, including the token, gets overridden.
• When Talawa-admin attempts to verify the token for user authentication through the API, it encounters an error stating that the user is not found.
• Redirected to orglist due to having the token and other credentials, but redirected back to the auth screen as the user was not found.
• And this results in a loop of redirecting from orglist to / and vice versa.
• Potential encryption method mismatches in the token cause API errors.

Solution Approach:

• Provide a unique prefix to the key stored in local storage to prevent conflicts.
• Implement functions (setItem, getItem, removeItem) for local storage.
• Use these functions instead of local storage directly, ensuring the key is prefixed and maintaining uniqueness in key names.

Outcome:

• Storing keys in local storage with unique names that are prefixed by a common string.
• This prevents conflicts between keys of similar types being stored in local storage.
• By avoiding conflicts of keys stored in local storage, the issue of continuous refreshing is resolved.

[beingnoble03]

@chandel-aman can you please explain the approach you have taken here? And, how does this fix the issue?

Sure!

Issue Explanation:

• The problem arises due to conflicts in data being stored by multiple projects on the same port in local storage.

Example Scenario:

• Running Talawa-admin on port 3000, logging in, and stopping the server without logging out causes data stored in local storage to persist.
• Running a different project (Project A) on port 3000, which also stores data in local storage, results in the Talawa-admin token being overridden by the Project A token.

Resulting Problem:

• When restarting the Talawa-admin server, it retrieves all data from local storage. However, some data, including the token, gets overridden.
• When Talawa-admin attempts to verify the token for user authentication through the API, it encounters an error stating that the user is not found.
• Redirected to orglist due to having the token and other credentials, but redirected back to the auth screen as the user was not found.
• And this results in a loop of redirecting from orglist to / and vice versa.
• Potential encryption method mismatches in the token cause API errors.

Solution Approach:

• Provide a unique prefix to the key stored in local storage to prevent conflicts.
• Implement functions (setItem, getItem, removeItem) for local storage.
• Use these functions instead of local storage directly, ensuring the key is prefixed and maintaining uniqueness in key names.

Outcome:

• Storing keys in local storage with unique names that are prefixed by a common string.
• This prevents conflicts between keys of similar types being stored in local storage.
• By avoiding conflicts of keys stored in local storage, the issue of continuous refreshing is resolved.

Ah, I see. I hope you have replaced all the localStorage functions with the utility ones. The PR looks good. Not sure why docs changes are being generated here.

[palisadoes]

The documentation issue was caused by a bug in our push workflow. This has been fixed.

1. @chandel-aman Please exclude the documentation files from the PR and merge with the upstream
2. @beingnoble03 is this approved excluding the documentation conflicts?

[chandel-aman]

Certainly, sir!

[palisadoes]

@chandel-aman

Merge with the latest upstream the documentation update bug has been fixed

[palisadoes]

@chandel-aman

Can we recommend a temporary fix to this issue by setting the default port for Talawa Admin to be something other than the default port 3000? That way the endpoint for Talawa Admin will more likely be unique?

[palisadoes]

The documentation file bug has been fixed.

1. Create a branch based on the current upstream
2. zip the files in the documentation directory for this branch
3. Use the PR branch
4. unzip the documentation over documentation directory
5. Commit the changes

Merging with your upstream should have no conflicts

[chandel-aman]

@chandel-aman

Can we recommend a temporary fix to this issue by setting the default port for Talawa Admin to be something other than the default port 3000? That way the endpoint for Talawa Admin will more likely be unique?

Yes, we could do that as a temporary fix, however that will depend on the port not having conflicting data in its localstorage. However, if we choose a port which is not used commonly this would avoid this issue from being triggered for the time being.

[chandel-aman]

The documentation file bug has been fixed.

1. Create a branch based on the current upstream

2. `zip` the files in the documentation directory for this branch

3. Use the PR branch

4. `unzip` the documentation over documentation directory

5. Commit the changes

Merging with your upstream should have no conflicts

Thank you!

[chandel-aman]

@palisadoes @beingnoble03 @aashimawadhwa @rishav-jha-mech
Could you please provide your review and merge this PR asap? Thanks!

[beingnoble03]

@chandel-aman @palisadoes this looks good. But, we'll have to ensure that everyone uses these functions instead of localStorage.xyz (which most of the contributors, unaware of these functions, would be using).

[chandel-aman]

@palisadoes
I tried updating the PR workflow file with these changes

- name: Set executable permissions
  run: chmod +x scripts/githooks/check-localstorage-usage.js

 - name: Check for localStorage Usage
    run: node scripts/githooks/check-localstorage-usage.js

And below is the script that checks the usage of localstorage in modified files.

I appreciate your guidance. It appears that the pre-commit hook is effectively detecting changes locally, but it's not identifying any modified files in the workflow. Could you please offer some guidance on where I might be encountering an issue? Thank you.

[palisadoes]

Create another script that scans the entire repo for use in the PR, or use the same script with a CLI flag to do so

[aashimawadhwa]

@chandel-aman please fix the conflicts

[chandel-aman]

The PR workflow is functioning as intended, checking for any usage of localStorage. Additionally, I've added a comment // SKIP_LOCALSTORAGE_CHECK. Including this comment in a file will instruct the hook to skip this check. This is necessary because a few tests weren't running without the usage of the localStorage function.

[palisadoes]

The PR workflow is functioning as intended, checking for any usage of localStorage. Additionally, I've added a comment // SKIP_LOCALSTORAGE_CHECK. Including this comment in a file will instruct the hook to skip this check. This is necessary because a few tests weren't running without the usage of the localStorage function.

Can you make those tests run correctly without the localStorage function? We don't want to have any legacy of this issue again.

[chandel-aman]

The PR workflow is functioning as intended, checking for any usage of localStorage. Additionally, I've added a comment // SKIP_LOCALSTORAGE_CHECK. Including this comment in a file will instruct the hook to skip this check. This is necessary because a few tests weren't running without the usage of the localStorage function.

Can you make those tests run correctly without the localStorage function? We don't want to have any legacy of this issue again.

Sir, I did try, but the current version of @testing-library/react (11.1.0) does not support mocking hooks. Additionally, the only files where this check has been skipped, other than the hook file itself and the pre-commit script, are test files, which will not affect this.
Link to the related issue and comment

[palisadoes]

@beingnoble03 @aashimawadhwa @rishav-jha-mech

Please review. The problem still occurs occasionally with the temporary fix on port 4321

[chandel-aman]

@aashimawadhwa @rishav-jha-mech @beingnoble03 Could you please review this PR?

[palisadoes]

Please fix the conflicting file. It was recently renamed.

[rishav-jha-mech]

@chandel-aman fix the failing test and conflicting PR. The code LGTM

[aashimawadhwa]

can you resolve the confllicts @chandel-aman, .github/workflows/pull-requests.yml has been recently renamed too

[chandel-aman]

@rishav-jha-mech @aashimawadhwa I've resolved the conflict and there are no failing tests. Could you please merge this PR?

[aashimawadhwa]

LGTM 👍🏼 Can be merged @palisadoes