8092 timestamp of data access request #9257

ericdevries · 2023-01-04T15:57:40Z

What this PR does / why we need it:

This adds a date column to the restricted file access request overview, indicating when the earliest request by that user was made.

It also fixes an issue where the request list was not updated when a request was approved or rejected.

To reproduce this bug (outside this branch) perform the following steps:

create a dataset with one or more restricted files
sign in with a different user that does not have permissions already to download the files
request access on the files
sign back in as the owner of the dataset
go to Edit Dataset > Permissions > Files
approve or reject the request
notice how the data table with the approve and reject options does not disappear or update when clicked.

Which issue(s) this PR closes:

Closes timestamp of data access request #8092

Special notes for your reviewer:

Suggestions on how to test this:

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Yes, it adds a date column to the restricted file access request overview. See screenshot below:

Is there a release notes update needed for this change?:

Yes, added.

Additional documentation:

None.

coveralls · 2023-01-04T17:09:48Z

Coverage: 20.167% (-0.02%) from 20.184% when pulling e2ddfd8 on ericdevries:8092-timestamp-of-data-access-request into 4903e9f on IQSS:develop.

pdurbin · 2023-01-04T17:19:32Z

@ericdevries thanks for the pull request! I added a release note, which you are welcome to tweak.

I took a quick look at the docs and I don't think we need any changes there.

API tests are passing. That's good.

You mentioned fixing a related bug. Can you please provide steps for reproducing that bug?

Finally, if you have a screenshot to share, you can put it under "Does this PR introduce a user interface change?" above.

I gave this a size of 10 because it involves a database update and a fair amount of code was changed. I put it in the community backlog. Thanks again!

ericdevries · 2023-01-04T17:31:36Z

I added the screenshot and the description on how to reproduce the bug. Let me know if you need anything else.

janvanmansum · 2023-01-06T16:06:32Z

@npoppelier: this PR should address your request for a date in file access request table.

@pdurbin : we cannot access the Jenkins build. Can you give more information about why it failed so @ericdevries can address it?

Thanks!

pdurbin · 2023-01-06T19:19:41Z

we cannot access the Jenkins build.

Yes, I'm sorry about this. It used to be open to the public but it's hard to do this securely. We can poke an hole in the firewall for an IP for you if you'd like.

Can you give more information about why it failed so @ericdevries can address it?

Please don't worry about it. The test that failed is simply flaky. I opened this issue so that we can address it separately:

flaky test: DownloadFilesIT.downloadAllFilesRestricted #9259

mreekie · 2023-01-10T22:05:17Z

Prio meeting with Stefano.

Moved from Community Backlog to ordered backlog

sekmiller · 2023-03-20T20:23:03Z

src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java

-            file.getFileAccessRequesters().remove(au);
+        boolean actionPerformed = false;
+        for (DataFile file : files) {
+            file.removeFileAccessRequester(au);


Should we test here to make sure that the removeFileAccessRequest was successful?

Sure, we do this on another place as well. I'll update it.

sekmiller · 2023-03-22T12:13:14Z

src/main/resources/db/migration/V5.13.0.4__8092-timestamp-of-data-access-request.sql

@@ -0,0 +1,2 @@
+ALTER TABLE fileaccessrequests
+ADD COLUMN IF NOT EXISTS creation_time TIMESTAMP WITHOUT TIME ZONE DEFAULT now();


Would it make sense to add a composite key to the table - though it might be problematic for upgrades, if there are any datafiles with multiple open requests from a given user

That was indeed the problem I faced while building this. If the migration scripts support queries as well, we could do some kind of group by trick and table renaming to remove duplicate rows and create a fresh table with only unique rows.

This will also require some changes in the code however.

So the question is, would something like this:

-- note this is pseudo-sql create table tmp_fileaccessrequests (user_id, file_id); insert into tmp_fileaccessrequests select distinct user_id, file_id from fileaccessrequests; drop table fileaccessrequests; rename table tmp_fileaccessrequests to fileaccessrequests;

be an acceptable solution?

Yes. That would work and ensure that we don't lose any data. Just modify the script to add this to the top then add the key at the end. Thanks!

sekmiller

Looks good overall. Just a couple of comments/questions.

Also, please update with the latest from the develop branch.

Thanks for the contribution!

sekmiller · 2023-03-22T14:18:18Z

@ericdevries , thanks for the PR. Just wanted to make sure you saw my questions/comments.

sekmiller · 2023-03-28T14:57:07Z

Hi @ericdevries just wanted to ask if you've had a chance to consider my questions/suggestions for this PR. Thanks!

mreekie · 2023-03-29T19:18:37Z

next sprint:

Review is done.
Waiting on input from originator of the pull request.
10 to 3 for this sprint

ericdevries · 2023-04-03T14:40:23Z

My apologies for the late reply, I did not see your questions. See my comments on your comments so we can further discuss what to do here

sekmiller · 2023-04-12T14:06:58Z

@ericdevries please make the update to the migration script that we discussed last week. Also make sure that you've updated this branch with the latest from our dev branch. Once those are done we can get this PR into QA. Thanks!

sekmiller · 2023-04-24T14:09:28Z

@ericdevries are you able to make the update I suggested for the migration script? If not, I can take it over.

…b.com/ericdevries/dataverse into 8092-timestamp-of-data-access-request

sekmiller

Changes as discussed have been made. I updated the migration script

seen in the Edit Permissions/File dialog when clicking the link with the # of files listed. In that dialog, things explode trying to get the directoryLabel of a FileAccessRequest (seen in log) and the popup dialog incorrectly shows the input to select a user, only has a grant button (no reject), and doesn't work (the last could be an issue with this branch only).

ericdevries added 3 commits January 4, 2023 15:05

Initial attempt

83d5ee8

Update attribute fixed to remove permission requests

be366a4

Add flyway migration script

7736680

pdurbin added Feature: Request Access Workflow Size: 10 A percentage of a sprint. 7 hours. labels Jan 4, 2023

add release note IQSS#8092

ec08035

pdurbin mentioned this pull request Jan 4, 2023

flaky test: DownloadFilesIT.downloadAllFilesRestricted #9259

Closed

sekmiller self-assigned this Mar 20, 2023

sekmiller reviewed Mar 20, 2023

View reviewed changes

Merge branch 'develop' into 8092-timestamp-of-data-access-request

364c7c7

sekmiller reviewed Mar 22, 2023

View reviewed changes

pdurbin assigned ericdevries Mar 23, 2023

mreekie added Size: 3 A percentage of a sprint. 2.1 hours. and removed Size: 10 A percentage of a sprint. 7 hours. labels Mar 29, 2023

Merge branch 'IQSS:develop' into 8092-timestamp-of-data-access-request

43ba648

Check for file access request removal before saving

d58f826

qqmyers added the GDCC: DANS related to GDCC work for DANS label Apr 6, 2023

qqmyers added this to the 5.14 milestone Apr 6, 2023

sekmiller added 3 commits April 26, 2023 09:52

Merge branch '8092-timestamp-of-data-access-request' of https://githu…

d15f1f4

…b.com/ericdevries/dataverse into 8092-timestamp-of-data-access-request

Merge branch 'develop' into 8092-timestamp-of-data-access-request

7bc678f

IQSS#8092 update migration script for primary key

e2ddfd8

sekmiller approved these changes Apr 26, 2023

View reviewed changes

sekmiller unassigned ericdevries and sekmiller Apr 26, 2023

kcondon self-assigned this Apr 26, 2023

kcondon merged commit cd54e8b into IQSS:develop Apr 26, 2023

qqmyers mentioned this pull request May 19, 2023

Fix Flyway script name #9602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8092 timestamp of data access request #9257

8092 timestamp of data access request #9257

ericdevries commented Jan 4, 2023 •

edited

Loading

coveralls commented Jan 4, 2023 •

edited

Loading

pdurbin commented Jan 4, 2023

ericdevries commented Jan 4, 2023

janvanmansum commented Jan 6, 2023

pdurbin commented Jan 6, 2023

mreekie commented Jan 10, 2023

sekmiller Mar 20, 2023

ericdevries Apr 3, 2023

sekmiller Mar 22, 2023

ericdevries Apr 3, 2023

sekmiller Apr 5, 2023

sekmiller left a comment •

edited

Loading

sekmiller commented Mar 22, 2023

sekmiller commented Mar 28, 2023

mreekie commented Mar 29, 2023

ericdevries commented Apr 3, 2023

sekmiller commented Apr 12, 2023

sekmiller commented Apr 24, 2023

sekmiller left a comment

		@@ -0,0 +1,2 @@
		ALTER TABLE fileaccessrequests
		ADD COLUMN IF NOT EXISTS creation_time TIMESTAMP WITHOUT TIME ZONE DEFAULT now();

8092 timestamp of data access request #9257

8092 timestamp of data access request #9257

Conversation

ericdevries commented Jan 4, 2023 • edited Loading

coveralls commented Jan 4, 2023 • edited Loading

pdurbin commented Jan 4, 2023

ericdevries commented Jan 4, 2023

janvanmansum commented Jan 6, 2023

pdurbin commented Jan 6, 2023

mreekie commented Jan 10, 2023

sekmiller Mar 20, 2023

Choose a reason for hiding this comment

ericdevries Apr 3, 2023

Choose a reason for hiding this comment

sekmiller Mar 22, 2023

Choose a reason for hiding this comment

ericdevries Apr 3, 2023

Choose a reason for hiding this comment

sekmiller Apr 5, 2023

Choose a reason for hiding this comment

sekmiller left a comment • edited Loading

Choose a reason for hiding this comment

sekmiller commented Mar 22, 2023

sekmiller commented Mar 28, 2023

mreekie commented Mar 29, 2023

ericdevries commented Apr 3, 2023

sekmiller commented Apr 12, 2023

sekmiller commented Apr 24, 2023

sekmiller left a comment

Choose a reason for hiding this comment

ericdevries commented Jan 4, 2023 •

edited

Loading

coveralls commented Jan 4, 2023 •

edited

Loading

sekmiller left a comment •

edited

Loading