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

Transfer data account #1319

Closed
ClaireLozano opened this issue Feb 13, 2017 · 20 comments
Closed

Transfer data account #1319

ClaireLozano opened this issue Feb 13, 2017 · 20 comments

Comments

@ClaireLozano
Copy link
Contributor

ClaireLozano commented Feb 13, 2017

Allow a user to transfer all this data to an account to another account (from different or same tenant).

Here is an quick overview of how everything works :

1 - User A generates a code with his 1st account
2 - He then logs in with his 2nd account, which is on another tenant (Note: it doesn't really matter how much time passes between step 1 and 2 ... can be hours, days, weeks,...)
3 - He enters the code in the appropriate field
4 - After verifying that the code is actually correct, all of documents linked to the 1st account is transferred to the new one ("transferred" means that we switch the rights/ownership from the 1st account to the 2nd one)
5 - For each document, if he isn't one of the owners, an email will be sent to the actual owner(s) to inform them of the potential issue so they can decide wether or not they want to keeping sharing the document with User A, even though he left his former tenant/institution.

Pull requests:

It's still a work in progress so there is some things (unit tests, optimization, ...) that need to be done...

@brecke
Copy link
Member

brecke commented Feb 13, 2017

Hello,

I was trying to look at your work on top of Hilary and since this is not yet a PR, I can't easily replicate it locally.

I have placed the oae-transfer folder inside Hilary and the folder inside of oae-transfer-front inside 3akai-ux but I'm probably missing something else. Can you drive me through it please?

Anyway, I recommend getting the PR as soon as possible (it doesn't matter if there is work to do still) because it will be easier to follow work progress, and will force you to tackle problems earlier (such as folder structure). Also, may I recommend you update your Hilary fork because it's 70 commits late. Thanks!

@brecke brecke added this to the Sprint #1 milestone Feb 13, 2017
@ClaireLozano
Copy link
Contributor Author

ClaireLozano commented Feb 13, 2017 via email

@ClaireLozano
Copy link
Contributor Author

I added the possibility to change the value of the "validity time" of the transfer request.

@brecke
Copy link
Member

brecke commented Feb 17, 2017

In what comes to the approach you describe above, there might be a conflict regarding the terms and conditions of OAE running instances that we are still analysing. As it stands right now,

The data belongs to the institution when said entity has adopted their tenancy and agreed to the instance's T&C

The data belongs to the user when the tenancy is not adopted, as indicated in the T&C users agree to for unadopted tenancies

We will get back to you on this as soon as possible.

@brecke
Copy link
Member

brecke commented Feb 20, 2017

Hi,

So, regarding the transfer procedure, here's the general policy we need to respect while designing this feature:

  1. The data belongs to the institution when said entity has adopted their tenancy and agreed to those conditions
  2. The data belongs to the user when the tenancy is not adopted, as indicated in the terms and conditions users agree to for unadopted tenancies

That being said, regarding your suggestion earlier:

all of documents linked to the 1st account is transferred to the new one ("transferred" means that we switch the rights/ownership from the 1st account to the 2nd one)

...this would need to be changed to "documents that the user is owner of - not just linked - and therefore have been created on his former tenancy", in case it's an unadopted tenancy.

In case it's an adopted tenancy, we feel that it might be best to create an admin setting for tenancy administrators to decide whether to allow users to transfer their own data (created inside that tenancy) if and when users leave the institution. If that setting is on, then the procedure should apply the same way as unadopted tenancies. If it's off, then no transfer should be possible.

I hope this has cleared it up. Don't hesitate to get in touch!

@ClaireLozano
Copy link
Contributor Author

Hi,

Thank you for the information, we think that is a good idea. Yesterday I made a push, so now the admin can decide if a user can make a transfer of his data or not.

Let me now if you see another things to change and maybe we should change some text in the user interface.

@brecke
Copy link
Member

brecke commented Feb 22, 2017

I'll wait for the pull request, as suggested earlier. Good job!

@ClaireLozano
Copy link
Contributor Author

I created the pull request but I have errors and I don't understand why...

@brecke
Copy link
Member

brecke commented Feb 23, 2017

Thanks for that, I'll take a look

@brecke
Copy link
Member

brecke commented Feb 27, 2017

The error in TravisCI is something you don't have to worry for now and does not affect functionality:

The command "test -d node_modules/webshot/node_modules/phantomjs-prebuilt" failed and exited with 1 during .

@brecke
Copy link
Member

brecke commented Feb 27, 2017

Hi,

Looking at this commit I'm guessing you tried to update your fork so that it included the most recent changes from the official Hilary repository, right? It turns out you used a merge instead of a rebase (see differences here).

I suggest you rebase your fork next time, because that's going to put your work "on top of" the most recent changes you fetch from upstream. Doing a merge will raise issues (check out this one) because we'll get duplicate commits on the main tree as soon as we merge ClaireLozano:transfer with master again. Next time follow the rebase flow as explained here.

In what comes to this particular situation, I can help you fix it if you grant me permissions to force push to your repository and we'll go from there. I checked out your repository by doing a git pull --rebase ....

Besides this, the feature is looking good! Some notes:

  • English translations missing
  • The process lacks flash messages with user feedback. For instance, let's say a user generates the code. The modal box disappears and there is no feedback information. It takes opening the modal window again to see the code. There should be feedback also during the transfer process (e.g. "Transfer will start now..." or similar, whatever makes sense).
  • Don't forget to protect edge cases such as picking the same e-mail address or having the same user transfer stuff to himself.

Good job!

@ClaireLozano
Copy link
Contributor Author

Hi,

Thanks for the explanation! I actually did a simple pull of the official Hilary repository, fixed the conflicts (conserving my work) and then I pushed it. I have added you as "collaborators" so you'll normally be able to make push.

About the front, sometimes the page is reloading I don't know why and if there is a message, it disappears because of the reloading... I'll take a look and try to fix it !

Thanks again for your help!

@brecke brecke modified the milestones: Backlog, Sprint #1 Mar 29, 2017
@ClaireLozano
Copy link
Contributor Author

Hi !

I pushed some modifications about this module. I’ve done the points that you told me :

  • english translation
  • cancel the reloading of the page + add notifications
  • protect edge cases

I also finished the unit tests and made some tests to make sure that the account transfer works on all possible case (for all types of documents, groups, all privacy, documents in folder, …).

@brecke
Copy link
Member

brecke commented Apr 28, 2017

Hey Claire,

I've tested it for a while and here are some findings:

  • I tried transferring an account and it failed, at least partially. Here's what I get on the terminal:
oae-hilary           | [2017-04-28T16:53:49.869Z] ERROR: oae/28 on 15c3ef12316f: An uncaught exception was raised to the application
oae-hilary           |     Error: Callback was already called.
oae-hilary           |         at /usr/src/Hilary/node_modules/async/dist/async.js:840:32
oae-hilary           |         at /usr/src/Hilary/node_modules/async/dist/async.js:3691:13
oae-hilary           |         at apply (/usr/src/Hilary/node_modules/async/dist/async.js:21:25)
oae-hilary           |         at /usr/src/Hilary/node_modules/async/dist/async.js:56:12
oae-hilary           |         at /usr/src/Hilary/node_modules/oae-transfer/lib/internal/dao.js:209:25
oae-hilary           |         at /usr/src/Hilary/node_modules/oae-transfer/lib/internal/dao.js:294:48
oae-hilary           |         at /usr/src/Hilary/node_modules/oae-authz/lib/api.js:277:17
oae-hilary           |         at /usr/src/Hilary/node_modules/oae-authz/lib/api.js:146:20
oae-hilary           |         at /usr/src/Hilary/node_modules/oae-authz/lib/api.js:197:16
oae-hilary           |         at /usr/src/Hilary/node_modules/oae-util/lib/cassandra.js:440:20

I think it might have to do with this piece of code:

_.chain(authzRoles).map(function(hash) { 
                _doUpdate(hash, idUserTarget, idUserOrigin, function (res){
                    if(res){
                        callback( null, [

In a nutshell, you're invoking callback a bunch of times.

  • The modal box one gets from clicking "Transfer account" asks the user to fill in "your another e-mail account". This implies that the user has a different e-mail account in both tenants, but it's not necessarily the case. If I'm reading it right, then the feature needs to cover all possible scenarios
  • If the user fills in the wrong e-mail address, the modal triggers a notification stating the transfer failed, but I feel the message should be more explicit an user-friendly. Probably something like "That code isn't valid for the e-mail ".

I'll proceed to a more in depth review as soon as these functional issues are addressed. Good work so far!

@ClaireLozano
Copy link
Contributor Author

Hi Miguel,

I never had this error before ... Can you tell me what kind of test did you make to produce this error ?
I think the problem comes from the version of Async module - maybe we have different version and it probably doesn't work with your’s - I rewrote the function without use Async. So you shouldn't have this error anymore.

You have all right for the modal box, I changed the text by “Enter the email of your other account…”. I also changed the text of the notification.

Please tell me if you see anything else !

@brecke
Copy link
Member

brecke commented Feb 23, 2018

I rewrote the function without use Async. So you shouldn't have this error anymore.

Nooooooo! 😆 Let's try async again, there must be something we can do, right?

In any case, is this ready to be reviewed? If so, let's make it a pull request.

@ClaireLozano
Copy link
Contributor Author

Wow ... I don't even remember this conversation x)

This is the pull request : #1326

@ClaireLozano
Copy link
Contributor Author

I finished to rebase to projects ! There are ready to be reviewed ;)

Hilary : #1326
3akai-ux : oaeproject/3akai-ux#4124
oae-rest : oaeproject/oae-rest#51

@brecke
Copy link
Member

brecke commented Mar 23, 2018

alright! good job! I'll review them as soon as possible

@brecke
Copy link
Member

brecke commented Dec 14, 2021

This issue will need to be redone from scratch after migrating to postgres (or any other relational database), which will allow for a completely different solution. It's still valid as a potential feature, just not the way it's been worked in this thread, so I'm keeping it on the backlog under a different name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants