FWM-Team / FWM-Backlog-Issues

This is a backlog of issues to deal with
0 stars 0 forks source link

Popups Discussion thread #3

Open FWAJL opened 9 years ago

FWAJL commented 9 years ago

Previous comments are here. Please add the new comments here.

FWAJL commented 9 years ago

@gsouvik

I've reviewed your code and I have a small comment before Brian and I give a full feedback. You have added in the active_list and inactive_list modules the $tooltip_message variable. These 2 modules are used in many places, not just the project and technician. What I saw is a PHP notice about an undefined variable, being tooltip_message, on the location/listAll page.

You need to check the variable before you load it.

See this example.

Do you understand the solution?

I have

gsouvik commented 9 years ago

@FWAJL Thank you for pointing that out. Yes, as the active_list module is used at other places as well I should have checked the existence of variable before referencing it. I'll keep it on my mind, next time I use a variable.

I see you've already patched the code, so I'll just pull the latest code from the feature_popup branch to get everything up-to-date.

Thanks again.

gsouvik commented 9 years ago

@FWAJL I just did a pull and I cannot login anymore. I see the login works through ajax requests. The response it is getting back is:

{ "result": 1, "message": "Logged in! Going to your projects... Please wait." }

Obviously the browser is failing to interpret this as a proper JSON due to the warning message and stops the login process. Please let me know what might be going wrong and how I may fix it. Thank you.

FWAJL commented 9 years ago

@gsouvik

My apologies. I think I didn't specify in the wiki about the database updates.

Look here to find the update scripts. This should solve you problem.

Thank you

gsouvik commented 9 years ago

@FWAJL Thanks, updated the database and it is working now.

FWAJL commented 9 years ago

@gsouvik

FYI, the updates scripts are used to perform small updates. The reset script is used to wipe all the data and start from scratch.

FWAJL commented 9 years ago

@gsouvik

I have made a small update to the Milestone C popup and I'm very happy how you made it work. It is easy to understand. Adding action is really simple!

I will push the code this weekend.

Very good work.

gsouvik commented 9 years ago

@FWAJL Thank you.

FWAJL commented 9 years ago

@gsouvik

I have committed the code. Have a look at the improvements.

Thank you.

FWAJL commented 9 years ago

@gsouvik

I have a few comments about the functions available in the code: can you please use $rq->getData($key) instead of $_GET[$key]? We try to avoid accessing the PHP global variables directly.

$rq is available in all controllers methods and you can pass it as a param to helper classes.

It is also true for $_POST and $_FILES which are found in the controller instance. For example, in a controller method: $_POST = $this->dataPost() and $_FILES = $this->files().

Thank you.

gsouvik commented 9 years ago

@FWAJL Thanks for pointing that out. While developing I was looking for a wrapper variable to access the GET superglobal, but could not find it. I didn't knew if it existed in any documentation, where I might have found and used it. Now I know that it's called "getData".

FWAJL commented 9 years ago

@gsouvik

Well, I wish I could have written a documentation as I built the application but Brian wants things done. Still, I documented a few methods and you can do to. It'll help us eventually.

Also, you can check if a GET param exists with $rq->getExists($key).