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

ATLAS-166: Allow more than own owner for a marker #89

Merged
merged 1 commit into from Jul 15, 2019

Conversation

@HelioStrike
Copy link
Collaborator

commented Jul 9, 2019

JIRA Issue: https://issues.openmrs.org/browse/ATLAS-166?jql=project%20%3D%20ATLAS

I created an auth table to define privileges of users.The user who created a marker is given 'all' (update, delete, add co-owners) privileges. Users having a privileges of 'update' (co-owners) can update the markers.

Screenshot from 2019-07-09 23-06-25

Screenshot from 2019-07-10 13-10-20

@HelioStrike HelioStrike requested review from bmamlin and cintiadr Jul 9, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch 5 times, most recently from 13805bf to 899ef95 Jul 9, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

I didn't realize that there already was an 'auth' table. I'll update the PR tomorrow morning.

@bmamlin
Copy link
Member

left a comment

I'm assuming that you've chosen to leave co-owner administration to creator and admins – i.e., co-owners cannot add/remove other co-owners. That's fine.

db/authtable.sql Outdated Show resolved Hide resolved
public/js/atlas.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
utils.js Show resolved Hide resolved

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch from 899ef95 to 24fa5c8 Jul 10, 2019

@HelioStrike HelioStrike requested a review from bmamlin Jul 10, 2019

package.json Show resolved Hide resolved
@bmamlin
Copy link
Member

left a comment

The current implementation of co-ownership is nice, but can be misleading, since the co-ownership changes are applied immediately and not when the save button is pressed (e.g., adding or removing co-owners and dismissing the bubble without saving does not revert the co-ownership changes.

One option would be to make the co-owner changes in memory and only apply them when the save button is pressed. This would require creating a mechanism for validating OpenMRS IDs independent of making auth rules and invoking multiple separate REST calls (saving marker + co-owner changes) when the save button is pressed.

Another option would be to move the ownership setting below the save button, making it more visually intuitive that ownership changes are independent of the save button. Depending on how it looks, we could consider collapsing the ownership section by default under a light grey "Advanced settings" label.

public/js/user.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch 5 times, most recently from 2333051 to b8057f8 Jul 11, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

@bmamlin I implemented 'advanced options', and made the changes. Please have another look. :)

@HelioStrike HelioStrike requested a review from bmamlin Jul 11, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch 2 times, most recently from d34c8ea to 8263388 Jul 12, 2019

@bmamlin
Copy link
Member

left a comment

The Advanced Options section looks really nice! I've made a couple small suggestions to address before we merge. Please don't forget to update the entire Dockerfile contents with what I provided; otherwise, this PR will break docker-compose builds (as alpine doesn't include python... a dependency for bcrypt).

public/js/user.js Outdated Show resolved Hide resolved
routes/auth.js Show resolved Hide resolved
public/js/user.js Outdated Show resolved Hide resolved
routes/auth.js Show resolved Hide resolved

@bmamlin bmamlin requested review from bmamlin and removed request for bmamlin Jul 15, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch from 8263388 to b5bb0e5 Jul 15, 2019

@HelioStrike HelioStrike requested a review from bmamlin Jul 15, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch from b5bb0e5 to 956dbbe Jul 15, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch from 956dbbe to 03cb370 Jul 15, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:ATLAS-166 branch from 03cb370 to 2d5a0f0 Jul 15, 2019

@bmamlin

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Looks good. While testing this PR, I discovered a few little issues, but I think there may have been introduced by other PRs, so I'm going to go ahead and merge this one and I'll post the issues I found to OpenMRS Talk.

@bmamlin bmamlin merged commit 78a0745 into openmrs:master Jul 15, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

Yup, the merge conflicts on this were pretty horrid. I think I might've messed something up while resolving conflicts. :|

@HelioStrike HelioStrike deleted the HelioStrike:ATLAS-166 branch Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.