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

[CB-163] Form resets when the Search button is clicked #152

Merged
merged 1 commit into from
Nov 6, 2017

Conversation

LWanjiru
Copy link
Contributor

JIRA TICKET NAME:

CB-163 - Form resets when the Search button is clicked

SUMMARY:

Currently, when a user (1) does a query about obs, (2) does a query about encounters, (3) views the results of the obs query, (4) clicks Back.
They see a form with the values entered in their previous search, which can be quite confusing.

After bug fix:
After clicking on 'Search', The data should be saved and the query reset.
Thus, a user should see an empty form upon clicking the ‘Search’ button. This means that even after viewing results, a user should view a reset form on clicking 'Back'.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling a4b55ab on LWanjiru:CB-163 into 997faaa on openmrs:master.

@@ -360,6 +360,7 @@ export default class ObsFilter extends React.Component {
<button
type="submit"
className="btn btn-success"
onClick={this.handleSubmit && this.handleReset}
Copy link
Contributor

@joel-ace joel-ace Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Just checked again. This might cause some unusual behaviours. One of these methods won't be called. Why don't you call the handleReset method from the handleSubmit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did implement it that way, at first, however, owing to the current structure of the codebase, when called within the handleSubmit method, it only executes the handleReset method, but the data in the query is not passed to the Search history.

I have tested this implementation and it works well, without causing any breaks or interfering with the flow. However, I would love to hear more in regard to the 'unusual behaviours' you suspect it may cause. In case I overlooked any of the edge cases. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding of the logical AND operator, the statement (this.handleSubmit && this.handleReset) will result to this.handleReset being called if this.handleSubmit evaluates to be true. I may be wrong though; I'll check more about this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That deduction is true. Currently, there is no implementations that prevents a search from executing, and even then, that would be handled within the this.handleSubmit, which would then be recalled until it satisfies all the requirements(edge cases) that would make the function collect all the required data in the form, and send it to the Database. When that transaction has been completed, then this.handleReset is called and executed, which is why you simultaneously see the results of the search in Search History and the query form is reset. (That is how I went about resolving it).

We did try several implementations with Cecilia before I settled on this implementation. However, I am open to pair program with you on this. I would love to redo it with you.

Copy link
Contributor

@joel-ace joel-ace Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was correct. The reason it works is that there's onSubmit={this.handleSubmit} somewhere on line 413. So this should just be onClick={this.handleReset} as this.handleSubmit && this.handleReset also evaluates to this.handleReset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know that, because I tried it. However, I welcome you to also try it out. As I said, I am open to discovering any other way to go about implementing this. I believe it would be more actionable if you did an implementation of the same, then we walk through it together.

Copy link
Contributor

@joel-ace joel-ace Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that this.handleReset is called first before this.handleSubmit so this resets the form before it is submitted. I'll advise you call this.handleReset from inside this.handleSubmit. Call it after line 107

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had tried that before too, but that was before I changed the flow. It definitely works, and does the exact same thing that I have executed here, as I realize, you gathered the same from the responses you got here https://stackoverflow.com/questions/47054134/why-does-the-logical-and-operator-call-these-two-functions-in-this-case. They explain the things I pointed out earlier.

Why do you feel that it should be implemented inside the submit function, rather than at the button which triggers the event?

Copy link
Contributor

@joel-ace joel-ace Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said in my previous comment, the way it is now, this.handleReset will be called first as it is bound directly to the button's onClick handler. This resets the form before it is submitted. You can log some data from both methods by placing a console.log() to check this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the best way forward is that we have a sync as I had proposed earlier, that way we can go over both perspective in detail. I will check your calendar and schedule sometime tomorrow so we can go through this, if that is okay with you.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling 9cd3c0b on LWanjiru:CB-163 into 85b8440 on openmrs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling 48bf332 on LWanjiru:CB-163 into 85b8440 on openmrs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.0% when pulling 48bf332 on LWanjiru:CB-163 into 85b8440 on openmrs:master.

Copy link

@andela-dbamidele andela-dbamidele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@joel-ace
Copy link
Contributor

joel-ace commented Nov 6, 2017

Looks Good

@Farhanisto
Copy link

looks good

@dkayiwa dkayiwa merged commit 3ce42de into openmrs:master Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants