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

Fixed add, delete, and edit markers functionality #45

Merged
merged 1 commit into from Jun 6, 2019

Conversation

@HelioStrike
Copy link
Collaborator

commented Jun 1, 2019

Fixed add, delete, and edit markers functionality. Here's a video of the atlas in its current state.

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-addeletedit branch from b5c8d10 to a566c68 Jun 1, 2019

@cintiadr cintiadr requested review from cintiadr, bmamlin and harsha89 Jun 2, 2019

@cintiadr

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Can you confirm to me if we do a validation on the backend that only the owner can modify their own sites?

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 2, 2019

@cintiadr My bad! The backend only checks whether the user is authenticated. However, the edit button for a marker wouldn't appear to a user if he isn't the owner. Would you like me to add a check in the backend to see whether the username dictated by a request is the same as that of the authenticated user?

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-addeletedit branch from a566c68 to bc42b35 Jun 2, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 2, 2019

@cintiadr I made the check by modifying the sql being executed when a modification request is called. Is that alright?

@cintiadr
Copy link
Contributor

left a comment

Look, I know nothing of javascript or frontend, so it looks fine.

@@ -119,6 +114,22 @@
userName = $('#user-name').val().trim();
userEmail = $('#user-email').val().trim();
if(currentUser != "/") {

This comment has been minimized.

Copy link
@cintiadr

cintiadr Jun 3, 2019

Contributor

I'm a little bit confused here, how is that possible that the currentUser would be '/'?

This comment has been minimized.

Copy link
@HelioStrike

HelioStrike Jun 3, 2019

Author Collaborator

@cintiadr When I printed out currentUser onto the console it turned out to be "/" when no user was logged in. Then again, I've no idea why it ended up like that in the first place. :/

This comment has been minimized.

Copy link
@cintiadr

cintiadr Jun 3, 2019

Contributor

Super weird. I wouldn't bother for now, but I suppose at some point you might discover why is that and try to clean that up.

Can you at least add a comment here? Maybe even a TODO comment.

This comment has been minimized.

Copy link
@HelioStrike

HelioStrike Jun 3, 2019

Author Collaborator

@cintiadr I just had a look at it. When no 'user' parameter is being passed to the page, value of hidden form element '#user-id' becomes "/" instead of "" as I had set it (maybe it is some representation of a blank string?). Should I set currentUser to "visitor" or something like that, if no user is logged in? Line 113 aims to do it I think, so maybe I edit my code to do that?

This comment has been minimized.

Copy link
@HelioStrike

HelioStrike Jun 3, 2019

Author Collaborator

@cintiadr I changed the code to set currentUser as "visitor" when the user is logged, as there are a lot of places in the code where "visitor" is being used. I tested stuff to see if everything worked and so far nothing seems to have broken.

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-addeletedit branch 2 times, most recently from aeec7f1 to b9dd689 Jun 3, 2019

public/js/user.js Show resolved Hide resolved

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-addeletedit branch 2 times, most recently from d44dc5a to 5cf5339 Jun 3, 2019

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

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-addeletedit branch 6 times, most recently from dcf9b0d to b49d0d6 Jun 3, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-addeletedit branch from b49d0d6 to 68fa42b Jun 4, 2019

@cintiadr

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

What needs to be done to merge this pull request? I'd rather not add new functionality to this one, but rather raise another PR later with any new features.

Is there any code improvements pending?

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

@cintiadr Since the improvements required changes to one of the 4 files, I pushed them in here. I'll make new PRs from now on. I don't think there are pending code improvements.

@bmamlin bmamlin merged commit 277e10b into openmrs:3.x Jun 6, 2019

@bmamlin

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I've tested this locally and it seems to be working, so merged. Thanks @HelioStrike!

@HelioStrike HelioStrike deleted the HelioStrike:3.x-addeletedit branch Jul 10, 2019

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