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.

Sign up for GitHub

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

Jump to bottom

Implement event agreement on event app #1904

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from
Draft

Implement event agreement on event app #1904

wants to merge 11 commits into from

Conversation

superryeti
Copy link
Contributor

@superryeti superryeti commented Mar 1, 2023 •
edited
Loading

Context:
On PR #1876, we added added models EventAgreement and EventAgreementSignatures. The goal of those models is to ultimately let Event Hosts attach EventAgreement to Events, and for the participants to read the event agreements and accept the event agreement before they are able to access the data.
This PR implements the Event Agreement on the Event APP for Hosts and Participants. Now Host is able to link and Event Agreement to an Event. The participants will then be required to accept the Event Agreement before they can access the data

Workflow of the feature

  1. Platform Admin creates Event Agreements from Admin console, Thanks to allow dua creation for events #1876. These Agreements will then be used by Event Hosts
  2. Event Hosts can choose from the list of existing Event Agreements when creating or editing the event.
  3. If an event has an agreement, the participants will be required to read and accept the agreement before they can access the data
    • First the participant request to join the event
    • The Event Host approves the request
    • The Participant then sees the section on Event detail page, that will ask them to sign the agreement.
    • The participant signs the event agreement
    • The participant gets access to the data

Questions

  1. Do we let event host change the Event Agreement from Event once someone has signed the agreement? If they change event agreement once people have signed the agreement then, things will get complicated(Project avoids problem by using immutable published project)
  • If the answer is yes, then i suppose we should be doing at least the following
    • Notify all the participants who had accepted the agreement, and ask them to agree to the new agreement.
    • Remove their access after a buffer time period or immediately?
  • If the answer is No, maybe we implement a feature that will prevent editing of Event after a certain date or maybe we can have a registration opens and closes date. So once registration opens, Event Host can't edit the event
  • or the third option, we trust the Event Host to not change the Event Agreement once someone has accepted the event(probably not a good design choice)

  1. Should I open a separate PR for tests? I see on project we have a test that says TestWorkflow and tests all the Project stuff from start to finish? If we want to add a test like that, it makes more sense to open a separate PR.
    Also, I did not see the project having a test for just signing DUA, so thought it might be a good idea to follow the same pattern

  2. Should Event Agreement be optional(when creating/editing event)?

  3. Do we let people unsign an agreement?

@superryeti superryeti marked this pull request as draft March 1, 2023 00:44
@superryeti
Copy link
Contributor Author

@alistairewj @tompollard

Could you please check the question section of the PR, when you have time?

I still have few small changes to make, so i marked this as draft, i will work on them tomorrow.

@superryeti superryeti force-pushed the au/event/feature/implement_event_agreement_on_event branch from a5cb337 to 376944f Compare March 1, 2023 17:07
@superryeti superryeti marked this pull request as ready for review March 1, 2023 17:13
@superryeti superryeti changed the title [WIP] Implement event agreement on event app Implement event agreement on event app Mar 1, 2023
@superryeti
Copy link
Contributor Author

Screenshots to help review easily

  1. Before participants registers to the event
    image

  2. After participants registers to the event
    image (1)

  3. After participants is approved to the event
    image (2)

  4. Sign Event Agreement page
    image (3)

  5. After the user signs the Event Agreement

image (4)

@superryeti
Copy link
Contributor Author

superryeti commented Mar 1, 2023 •
edited
Loading

@tompollard @alistairewj , Ready for review.

There are few things that might need to be implemented pending your answers to the question on the PR(please check PR description)

@bemoody
Copy link
Collaborator

bemoody commented Mar 2, 2023

Do we let event host change the Event Agreement from Event once someone has signed the agreement?

I'm not sure whether you mean to say modifying an agreement, or switching an event to point to a different agreement.

The agreement is a fixed object and nobody should ever change it. If changes are needed then we should create a new agreement, and point the event to the new version.

It seems unlikely that we would ever want to do this after an event is public. If so, I think we'd want to say that yes, anybody who agreed to the first version of the agreement will now be required to click through the second version.

Should I open a separate PR for tests?

No. Please write test cases for a new feature along with the new feature.

I see on project we have a test that says TestWorkflow and tests all the Project stuff from start to finish? If we want to add a test like that, it makes more sense to open a separate PR.

A single "comprehensive" test case tends to be a lot of work to write and also tends not to exercise all of the functionality. Instead, try to write small and focused test cases. Test a single request at a time if you can. Use fixtures to define the initial state, make a request, and verify that it has the expected effect.

Should Event Agreement be optional(when creating/editing event)?

Yes.

Do we let people unsign an agreement?

No.

@superryeti
Copy link
Contributor Author

superryeti commented Mar 2, 2023 •
edited
Loading

@bemoody

Thank you for the feedback

I'm not sure whether you mean to say modifying an agreement, or switching an event to point to a different agreement.

---> Here i meant to ask mainly about switching. Noted about the expectation that admin's will not edit the existing Agreement.

I suppose my question was more of
In case of switching of agreement linked to an event, should we trust the Host to not make switch or should we enforce it so that they are not able to make it even if they want.

If i understood correctly, your answer was yes let's enforce it, right?

@bemoody
Copy link
Collaborator

bemoody commented Mar 2, 2023

Event hosts are not necessarily empowered to make decisions about who can access what data under what conditions.

Adding temporary database access to an event is something that in general has to be approved by the data contributors, and the data use agreement is (or might be) part of that.

So... event agreement should be tied to event dataset, I guess?

@superryeti
Copy link
Contributor Author

superryeti commented Mar 2, 2023 •
edited
Loading

Event hosts are not necessarily empowered to make decisions about who can access what data under what conditions.

Adding temporary database access to an event is something that in general has to be approved by the data contributors, and the data use agreement is (or might be) part of that.

So... event agreement should be tied to event dataset, I guess?

@bemoody I think that makes sense.

Currently an Event can have multiple Datasets attached to it. so i am having difficulty wrapping my mind around how to tie the Event Agreement to dataset(s) or that the implementation would probably be the same like what we have now[which is Event has dataset(s), and there will be 1 agreement created for that, and that agreement will have the data use agreement which covers all the dataset(s)]

As no matter how many number of dataset we have in the event, we would probably write all the agreement stuff for those dataset under that event in 1 single event agreement.

Or was your suggestion, that maybe we shouldn't let event host change the agreement or assign it at all? Maybe Platform Admin or whoever has the permission should be do it.

@superryeti superryeti force-pushed the au/event/feature/implement_event_agreement_on_event branch 3 times, most recently from 48311de to b0ed5bd Compare March 6, 2023 17:59
@superryeti
Copy link
Contributor Author

I added the tests for signing the event agreement, and updated the model to make event agreement optional.

For the part about requiring participants to resign the event agreement, I will open another PR after this one.

@superryeti superryeti marked this pull request as draft March 7, 2023 22:53
@superryeti
Copy link
Contributor Author

superryeti commented Mar 7, 2023 •
edited
Loading

Currently paused while we discuss/figureout how we want to implement access to datasets/project

@superryeti
Add event_agreement to Event Model
1907357
@superryeti
Add event_agreement to Event Form
70fa1c1 
This should let us select active Event Agreement during Event Creation or Edit
@superryeti
update EventAgreement Model __str__ method
aaf9d03 
The Option displayed on the `EventForm` for the field `event_agreement` of model `Event` depends on `__str__` method of `EventAgreement`. Since we can have different version of same `EventAgreement`, it is a good idea to show the `EventAgreement` version to the Event Host.

Before the change, if there are 2 Event Agreement with Name "My Event Agreement" but different versions, then the Event Host will see the same name repeated twice which will be confusing.

PS: i also checked how DUAs are implemented in project, and i think the expectation there might be that the Administrator will set the old DUAs to inactive.

So this commit might be optional if we assume
1. Old version of event Agreement will not be used by Event Host.
2. Platform Administrator will set the old versions of Event Agreement to inactive.
@superryeti
update has_access method to check for event_agreement
3be4360 
Note: Although we have plans to refactor this method in the future, i had to update it for now because we dont know how we will refactor it later. Since this change is required for participant to access the dataset through events, it should be okay to update it here. The logic will get refactored when we refactor the function.
@superryeti
refactor has_access to allow event host to skip access checks
3060dcc 
This change assumes that EventHost dont need to sign the EventAgreement.

On this commit, i added  a separate if statement to provide access to event host(the alternative was to have the same check in two different if statement with and)
@superryeti
Add the Event Agreement section on the Event Detail page
e9c351d 
The workflow is that once participants are approved for the Event, they will have to sign the EventAgreement(it doesnot make sense to ask people to sign the agreement if they haven't been accepted to the event as we might end up with people who have signed the event agreement but were rejected from event)
@superryeti
update dataset table to work with Event Agreement
330f4f8 
Changes
1. Fix typo Archieved > Archived
2. Add a conditional statement to check if the user has signed the Event Agreement.

The block of if elif statement maintains the expect flow i.e users are expected to
1. First request participation
2. once approved, sign the agreement
3. and then get access to the dataset
@superryeti
allow participants to sign the event agreement
244ed39
@superryeti
fixed failing tests
db29646 
Event creation/edit test were failing because the event form expects and `event_agreement` field.
@superryeti superryeti force-pushed the au/event/feature/implement_event_agreement_on_event branch 2 times, most recently from fc5c9a0 to adf7d27 Compare March 15, 2023 16:22
@superryeti
add test for events agreement
7c7b9b3 
Added two tests
1. Edits event to switch the event agreement
2. Participant joins an event and sings the agreement
@superryeti
add migration file
9899c28
@superryeti superryeti force-pushed the au/event/feature/implement_event_agreement_on_event branch from adf7d27 to 9899c28 Compare March 15, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@superryeti @bemoody