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

[Doc Repo] Refactor to use new File Uploader class #5234

Merged
merged 30 commits into from
Jul 8, 2020
Merged

[Doc Repo] Refactor to use new File Uploader class #5234

merged 30 commits into from
Jul 8, 2020

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Sep 18, 2019

Brief summary of changes

  • Use the new File Uploader class to do file uploading.
  • Add a new permission document_repository_edit to separate view and edit permissions.
  • Cleanup files class in the doc repo module

Testing instructions (if applicable)

  1. Run the patch.
  2. In the Configuration module under Paths, set documentRepositoryPath to a writable folder on your system.
  3. Using a superuser, or another user with the new document_repository_edit permission, ensure that you can upload files.
  4. Make sure that you can download the files you've uploaded.
  5. Try uploading a file type that's not allowed, such as an .mp3 file. You should get a Sweet Alert error message.
  6. Remove the document_repository_edit permission. You should get a 403 response when trying to upload any file.

Links to related tickets (GitHub, Redmine, ...)

@johnsaigle johnsaigle added Cleanup PR or issue introducing/requiring at least one clean-up operation Category: Security PR or issue that aims to improve security State: Needs documentation PR that needs a more exhaustive documentation to proceed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Sep 18, 2019
@johnsaigle
Copy link
Contributor Author

This is accidentally based on another PR... will rebase soon.

Also I didn't update the test plan but I imagine that needs to be done.

@johnsaigle johnsaigle added Language: SQL PR or issue that update SQL code and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs documentation PR that needs a more exhaustive documentation to proceed labels Sep 18, 2019
@@ -101,7 +101,8 @@ INSERT INTO `permissions` VALUES
(52,'instrument_manager_write', 'Instrument Manager: Install new instruments via file upload', 2),
(53,'publication_view', 'Publication - Access to module', 2),
(54,'publication_propose', 'Publication - Propose a project', 2),
(55,'publication_approve', 'Publication - Approve or reject proposed publication projects', 2);
(55,'publication_approve', 'Publication - Approve or reject proposed publication projects', 2),
(56,'document_repository_edit','Upload and edit files in Document Repository','2');
Copy link
Contributor

@jesscall jesscall Oct 23, 2019

Choose a reason for hiding this comment

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

Are you proposing 3 doc repo permissions? Is this new document_repository_edit permission also separate from the document_repository_delete permission?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to separate view from edit, basically.

I'm not sure whether there should also be a delete. Maybe there is a use case where someone is allowed to edit information about doc repo files but shouldn't be able to remove them entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with separating view and upload / edit.

I'm not sure we need that much granularity in terms of edit / delete, but I'm not strictly opposed to it

@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Oct 23, 2019
@jesscall jesscall added the State: Needs RB update PR that needs to update Raisinbread to reflect its changes label Oct 23, 2019
@johnsaigle johnsaigle removed State: Needs RB update PR that needs to update Raisinbread to reflect its changes State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Oct 28, 2019
@johnsaigle
Copy link
Contributor Author

TODO: Restart Travis

@johnsaigle johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Oct 30, 2019
@johnsaigle johnsaigle added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed labels Nov 27, 2019
@johnsaigle johnsaigle added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Dec 2, 2019
@johnsaigle
Copy link
Contributor Author

#5815 also adds a document repository setting and I think that PR should go in before this one. Adding Blocked label.

@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Dec 2, 2019
@zaliqarosli
Copy link
Contributor

@johnsaigle is this ready for review even though needs rebase and is blocked?

@johnsaigle
Copy link
Contributor Author

@zaliqarosli Nope it will require some changes after all these months. I'll fix it soon.

@johnsaigle johnsaigle added State: Needs RB update PR that needs to update Raisinbread to reflect its changes and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Dec 3, 2019
@johnsaigle
Copy link
Contributor Author

The raisinbread files got messed up during my rebase. I'll rerun the exporter script and commit the correct results when the VMs come back.

@johnsaigle johnsaigle changed the base branch from major to master December 3, 2019 18:44
@johnsaigle
Copy link
Contributor Author

@nicolasbrossard It's working now in my local testing. Would you mind trying again?

@nicolasbrossard
Copy link
Contributor

Works now! 👍 Approving this PR.

@johnsaigle
Copy link
Contributor Author

Thanks for all the help!

@johnsaigle johnsaigle requested a review from driusan July 7, 2020 21:27
@driusan driusan merged commit f38c95d into aces:master Jul 8, 2020
@ridz1208 ridz1208 added this to the 24.0.0 milestone Jul 15, 2020
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
-  Use the new File Uploader class to do file uploading.
- Add a new permission document_repository_edit to separate view and edit permissions.
- Cleanup files class in the doc repo module

    Resolves aces#4854
    Resolves aces#6373
@ridz1208 ridz1208 mentioned this pull request Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Category: Security PR or issue that aims to improve security Cleanup PR or issue introducing/requiring at least one clean-up operation Critical to release PR or issue is key for the release to which it has been assigned Language: SQL PR or issue that update SQL code Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
9 participants