Configiuration settings for advertisement server added under Yioop. In this, user can add location of advertisement on search results page. Also, user can add ad script (Advertisement banner) in settings. Script would execute on search results page which displays advertisement banner.

Steps to reproduce: New Functionality added.

Hey Pushkar, What's an ETA on a patch with the revisions from Tuesday?

Best, Chris

Hello Dr. Pollett,

I have added updated patch. Please review and suggest changes, if any.

Patch filename: adserver_config_v2.patch

Thanks & Regards, Pushkar

Hey Pushkar,

Thanks for your latest patch. Unfortunately, your patch is generating a lot of whitespace errors (more than 30 + it has a lot of stray tab characters). Can you clean them and fix the indentation and make a new patch? Here are a few other things I noticed.

If you have a long line with an assignment it should look like: x = lots of stuff +

   more stuff;

(four spaces) not x = lots of stuff + more stuff;

The code in showHideScriptdiv() could easily be shortened using a loop.

You have checks like: $data['AD_LOCATION'] == 'Side' Either $data['AD_LOCATION'] is not localized, which is bad, or 'Side' is effectively a constant. If it is a constant, it should be in all caps 'SIDE'. It is also preferable to use the const keyword.

You seem to be trying to indent too much in places in serversettings_element.php and the indentation seems to be also messed up in renderSearchResults.

In profile_model.php $profile_fields should be alphabetical.

ADLOCATION should be AD_LOCATION

Best, Chris

Thank you for your comments. I will update patch soon.

Thanks & Regards, Pushkar

Make sure when you post a revised patch to also post add a note to the issue. Notes to the issues generate emails but patch additions don't seem to.

Best, Chris

Hello Dr. Pollett,

I have added updated patch. Please review and suggest changes, if any.

Patch filename: adserver_config_v3.patch

Thanks & Regards, Pushkar

Hello Dr. Pollett,

I have added updated patch. Please review and suggest changes, if any.

Patch filename: adserver_config_v4.patch

Thanks & Regards, Pushkar

Hello Dr. Pollett,

I have added updated patch. Please review and suggest changes, if any.

Patch filename: adserver_config_v5.patch

Thanks & Regards, Pushkar

Hey Pushkar,

Did you do a git pull before creating your patch? Your patch does not apply.

Chris

Hello Dr.Pollett,

I did git pull before creating patch. I have not observed any conflicts while creating patch.

Regards, Pushkar

My bad. I think I download v2 by accident.

Chris

Hey Pushkar,

I am trying to clean up your code now, but will probably accept this patch. When you see it go live, look for the commit immediately afterward by me and do a git diff to see how I changed it. The HTML your code is outputting is not accessible -- controls should have labels and your radio buttons don't. You have a label tag before it, but it has no nested tags, and it doesn't have a for attribute. Also, you shouldn't be doing comparisons to translated values -- that's very unsafe.

Chris

Another thing don't use tags without corresponding close tags.

I.e., write [br /] (pretend square brackets are less than greater than) or [br][/br] not just [br]

Hello Dr.Pollett,

Thank you for your comments. I will definitely check code revision. I will make sure that I will cover these suggestions here onwards.

Sorry for the indentation issues you faced. I misinterpreted guidelines. I assumed that will have to indent four spaces only when there are control statements and iterations. From comment, it looks like I will have to add spaces for every second line.

Just a question, radio button labeling problem you faced at your side, is it because of locales are missing?

Thank you again.

Regards, Pushkar Umaranikar

String you use for translation should follow the format: [simplified name of file]_[rough_english_translation] For example, rather than ad_element_none serversettings_element_none

Your code doesn't seem to work correctly for mobile platforms or right-to-left languages. You have to make sure to actually test these before submitting patches. Most browsers have a built in way to set the User-agent so you can set it to some kind of phone. That way you can test mobile from your laptop. Similarly, switch the language to arabic to test for a right-to-left language.

Hey Pushkar,

Congratulations! Your code is now of the live site. Please compare your patch with commit 12e2a10 to see how I handle mobile and right-to-left languages and how I wanted formatting and strings.

Best, Chris

First pass done by 12e2a10

Attachments:

1600.patch

adserver_config_v2.patch

adserver_config_v3.patch

adserver_config_v4.patch

adserver_config_v5.patch