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

Editable mode & purpose confirm #508

Merged
merged 9 commits into from
Jan 7, 2019
Merged

Conversation

atton16
Copy link
Contributor

@atton16 atton16 commented Jan 2, 2019

No description provided.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@shankari
Copy link
Contributor

shankari commented Jan 6, 2019

@atton16 Your only commit 3e0afe3 basically includes html file changes alone (it looks like you add a second button in addition to the one that @sunil07t had already added to open the mode and purpose popovers).

However, this does not address the primary issue with the previous PR, namely that it will ONLY pull data from the usercache ($window.cordova.plugins.BEMUserCache.getAllMessages) and it will NOT read mode and purpose values that have already been pushed to the server.

I highlighted this in the issue (https://github.com/e-mission/e-mission-phone/issues/503#issuecomment-443484106) and suggested how to fix it; however, I don't see the fix in your commit.

Did you test this with server pushes? Can you please fill out the testing done, along with screenshots of the change? Similar to the initial PR (#307).

@shankari
Copy link
Contributor

shankari commented Jan 7, 2019

I should clarify that I am not opposed to merging this as an intermediate step as long as you make the final changes later. But I just want to clarify that as far as I can see, this will not be a complete fix. For example, if you confirm your trips on the 2nd, and go back to them on the 4th, with this PR, you will not see the confirmed mode and purpose.

@atton16
Copy link
Contributor Author

atton16 commented Jan 7, 2019

We knows this limitation and we are working on it. The reason that we did not use the command you gave me is that our programmer reported that the command not found so we will investigate into that matter after this.

@shankari
Copy link
Contributor

shankari commented Jan 7, 2019

The reason that we did not use the command you gave me is that our programmer reported that the command not found so we will investigate into that matter after this.

https://github.com/e-mission/e-mission-phone/blob/master/www/js/services.js#L232

@shankari
Copy link
Contributor

shankari commented Jan 7, 2019

We knows this limitation and we are working on it.

ok, I am merging this for now. @asiripanich, bewarned while testing...

@shankari shankari merged commit b28a8a9 into e-mission:rk-unsw Jan 7, 2019
@shankari
Copy link
Contributor

shankari commented Jan 7, 2019

@atton16 @asiripanich deployed, should be live in ~ 10-20 mins

@asiripanich
Copy link
Member

@shankari Thanks heaps!

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

Successfully merging this pull request may close these issues.

6 participants