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

Standardizing Asset Directory Structure #1449

Closed
wants to merge 12 commits into from
Closed

Standardizing Asset Directory Structure #1449

wants to merge 12 commits into from

Conversation

zakhaev26
Copy link

Description

This PR addresses the issue related to the inconsistent storage of image and video assets in the Talawa API repository. The goal was to standardize the directory structure to align with the one used in Talawa Admin.

Changes Made

  1. Moved image and videos assets from the root directory to src/assets/image and src/assets/video following the structure used in Talawa Admin.
  2. Updated markdown files in the root directory to reference the new asset locations under src/assets.
  3. Deleted assets files from the root directory that were not referenced in markdown files at talawa-api .
  • Currently the entire codebase didn't have any reference of assets present in videos directory ,so I removed it.
    Screenshot from 2023-12-03 10-00-09
  1. Configured .gitignore to prevent future uploads to video/ and image/ directories and ensure these directories remain in the repository.
  2. Checked and updated objects in markdown files to reference the new directory structure.
  • Current Structure :
    image

Links

Additional Notes

  • This PR is part of the effort to maintain a consistent and organized asset directory in the codebase across Talawa repositories.

Potential Impact

Developers working on the Talawa API should be aware of the changes in asset locations.

Closes: #1441

Copy link

github-actions bot commented Dec 4, 2023

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

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

We need an answer for this question here before we can approve this PR. The correct file locations need to be verified. We MUST NOT put files in an inappropriate location.

@zakhaev26
Copy link
Author

Please take a look.

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

The Gitignore is incorrect.

.gitignore Outdated Show resolved Hide resolved
@palisadoes
Copy link
Contributor

Not if they are unused elsewhere

…`video` directory,fixes links in `README.md` `INSTALLATION.md` , removes redundant files.
@zakhaev26
Copy link
Author

zakhaev26 commented Dec 5, 2023

PTAL;I changed the gitignore file

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 70 lines in your changes are missing coverage. Please review.

Comparison is base (c0468a4) 98.17% compared to head (2f559c3) 97.61%.
Report is 92 commits behind head on develop.

❗ Current head 2f559c3 differs from pull request most recent head bfa329d. Consider uploading reports for the commit bfa329d to get more accurate results

Files Patch % Lines
src/utilities/PII/decryption.ts 0.00% 17 Missing ⚠️
src/resolvers/middleware/currentUserExists.ts 45.83% 13 Missing ⚠️
src/utilities/PII/encryption.ts 0.00% 13 Missing ⚠️
src/utilities/PII/isAuthorised.ts 0.00% 11 Missing ⚠️
src/resolvers/Mutation/createPost.ts 86.95% 6 Missing ⚠️
...tilities/encodedVideoStorage/uploadEncodedVideo.ts 96.29% 3 Missing ⚠️
src/resolvers/Mutation/removeAdvertisement.ts 92.85% 2 Missing ⚠️
...c/resolvers/Query/postsByOrganizationConnection.ts 33.33% 2 Missing ⚠️
src/resolvers/Subscription/onPluginUpdate.ts 0.00% 2 Missing ⚠️
src/resolvers/Query/postsByOrganization.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1449      +/-   ##
===========================================
- Coverage    98.17%   97.61%   -0.57%     
===========================================
  Files          184      214      +30     
  Lines        10767    12906    +2139     
  Branches       835     1018     +183     
===========================================
+ Hits         10571    12598    +2027     
- Misses         186      293     +107     
- Partials        10       15       +5     

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

@palisadoes palisadoes requested a review from noman2002 December 5, 2023 15:22
@palisadoes
Copy link
Contributor

@noman2002 Comments and / or suggestions please

Copy link
Member

@noman2002 noman2002 left a comment

Choose a reason for hiding this comment

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

Changes looks good to me, but instead of mentioning each file in gitignore we can have separate folder for the images we need in the project.

@zakhaev26
Copy link
Author

instead of mentioning each file in gitignore we can have separate folder for the images we need in the project.

won't that allow commits to images folder ?
As I was asked that the directory (images,videos in root ) needs to be maintained , but no files can be uploaded in that folder using git.

@noman2002
Copy link
Member

Changes looks good to me, but instead of mentioning each file in gitignore we can have separate folder for the images we need in the project.

@palisadoes Your thoughts on this.

.gitignore Outdated Show resolved Hide resolved
@palisadoes
Copy link
Contributor

  1. Please refer to the GitIgnore tutorial site. It has examples of how to ignore the contents of directories without specifying individual files
    1. https://git-scm.com/docs/gitignore
  2. If the files in the image/ and videos/ directories are not referenced in the code base, then they should be removed.
  3. If the image/ and videos/ directories are required by the code base, then they should remain, else they should be removed.

@zakhaev26
Copy link
Author

zakhaev26 commented Dec 6, 2023

If the image/ and videos/ directories are required by the code base, then they should remain, else they should be removed.

The image/ and videos/ directory is not required in the codebase now ,as they have been shifted to the public directory.
Redundant files have been already deleted and all the references to these assets have been changed and are valid too.
In that case,is it necessary to keep the these image/ and videos/ folder?

A simple image/* and videos/* line in gitignore would do the work of not letting anyone commit files in these folders.

@palisadoes
Copy link
Contributor

@zakhaev26 Where are image and video files stored for posts from the Mobile app and Talawa-Admin? We need to make sure that these actions will still work. At some time in the past they were stored in these directories. Double check before removing them.

@zakhaev26
Copy link
Author

PTAL,did changes as requested by @EshaanAgg . Please let me know if the approach is wrong/needs more changes

@zakhaev26 zakhaev26 requested a review from EshaanAgg December 7, 2023 20:47
Comment on lines +36 to +39
# Ensuring no images and video are commited in ./video and ./image directories


markdown/*
Copy link
Contributor

@EshaanAgg EshaanAgg Dec 7, 2023

Choose a reason for hiding this comment

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

Why have you added the markdown directory here? I don't think so this directory exists.
It should contain the videos and images directories that get the uploaded files.

Copy link
Contributor

Choose a reason for hiding this comment

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

You also want to make sure that any files in any subdirectories of those two directories don't get uploaded. This syntax won't do that

@palisadoes
Copy link
Contributor

  1. Please merge with the latest upstream and commit. That should remove the errors.
  2. We need to get this implemented. The outcome will be used in 2024 for some issues in the pipeline

@palisadoes
Copy link
Contributor

Is this ready for review?

@palisadoes
Copy link
Contributor

Closing due to inactivity

@palisadoes palisadoes closed this Jan 1, 2024
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.

Standardize the location of repository assets
4 participants