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

non site based reporting #58

Open
drfaustusfade opened this issue Feb 17, 2021 · 55 comments
Open

non site based reporting #58

drfaustusfade opened this issue Feb 17, 2021 · 55 comments

Comments

@drfaustusfade
Copy link
Owner

to check if we can have a location without site_id and site_name

@drfaustusfade
Copy link
Owner Author

to check if validation can be set per country

@drfaustusfade
Copy link
Owner Author

to check if titles without site_name will be displayed correctly

@fakhrihawari
Copy link
Collaborator

to check if titles without site_name will be displayed correctly

Screen Shot 2021-02-18 at 09 25 54

Just Like this?

@fakhrihawari
Copy link
Collaborator

to check if validation can be set per country

I think we can do that

@fakhrihawari
Copy link
Collaborator

to check if we can have a location without site_id and site_name

to check if we can have a location without site_id and site_name

In model site_name is required

@drfaustusfade
Copy link
Owner Author

alright, let's try there were already such questions and requests for ET and NG for non-site reporting and optional site name, I think we can have that feature
please try to implement it

@fakhrihawari
Copy link
Collaborator

fakhrihawari commented Feb 19, 2021

Mock up

sitenameno.mp4

@drfaustusfade
Copy link
Owner Author

Looks alright, what do you think also if visually having some kind of greyed out input, but on hover input would be active

could you also please push commit, with not mandatory site name input for ET to have a test run

@drfaustusfade
Copy link
Owner Author

For engine and frontend, please confirm also that having site name as non mandatory would not break anything

@fakhrihawari
Copy link
Collaborator

but i'm not done NG coz NG have different form

@drfaustusfade
Copy link
Owner Author

Alright, we will try to migrate them later, when infinite scroll is done

@fakhrihawari
Copy link
Collaborator

ok

@fakhrihawari
Copy link
Collaborator

fakhrihawari commented Feb 19, 2021

@drfaustusfade
Copy link
Owner Author

looks good,
rename: No need to fill this => "-"
Site Name? => Site
SiteName => Site?
is required site_name should be removed on other collections in addition to TargetLocation? will that cause issues?

Note: as for now NG opted to still have required

let's have that feature for future use, but we will use just not mandatory site_name input for now so that not to confuse partners

  • to check not required site_name input for ET without a checkbox

@fakhrihawari
Copy link
Collaborator

So for NG no need to change ?

@drfaustusfade
Copy link
Owner Author

no need for now, but they can decide later to have that feature

@fakhrihawari
Copy link
Collaborator

without checkbox
https://user-images.githubusercontent.com/12655589/108685233-7b228900-7526-11eb-89b9-54b45c24add4.mp4

If location do not have site_name it not appear in monthly report

@drfaustusfade
Copy link
Owner Author

what is the reason?

@fakhrihawari
Copy link
Collaborator

what is the reason?

i dont know

@drfaustusfade
Copy link
Owner Author

alright, let's check, what are branches?

@fakhrihawari
Copy link
Collaborator

I found the problem, the site_name is set to undefined if its blank , i will fix it

@fakhrihawari
Copy link
Collaborator

Updated with test to check not required site_name input for ET without a checkbox and bug fix monthly report location without sitename

@drfaustusfade
Copy link
Owner Author

alright, would setting site_name defaultsTo "" fix also the problem? let's set it like that to safeguard. Also other collection required site_name would inherit from target_locations?

@drfaustusfade
Copy link
Owner Author

NG still have SiteName selection

@fakhrihawari
Copy link
Collaborator

alright, would setting site_name defaultsTo "" fix also the problem? let's set it like that to safeguard. Also other collection required site_name would inherit from target_locations?

yes, i change the model for beneficairies and location for report like target location coz it will not appear in monthly report

okay updated and remove hide sitename selection from NG

@drfaustusfade
Copy link
Owner Author

alright, some issues
that on adding location for ET, the site name is required
and if removing the site name from target location, the site name will still be with old name

@fakhrihawari
Copy link
Collaborator

fakhrihawari commented Feb 23, 2021

alright, some issues
that on adding location for ET, the site name is required
and if removing the site name from target location, the site name will still be with old name

actually i already fix this before, i comment the code that solve for that issue coz i think site_name defaultsTo "" will also fix that too and not to checki it again . But now it fix

fixbuget.mp4

@drfaustusfade
Copy link
Owner Author

ok, perfect, adding monthly report locations also updated?

alright, some issues
that on adding location for ET, the site name is required
and if removing the site name from target location, the site name will still be with old name

actually i already fix this before, i comment the code that solve for that issue coz i think site_name defaultsTo "" will also fix that too and not to checki it again . But now it fix

fixbuget.mp4

@fakhrihawari
Copy link
Collaborator

ok, perfect, adding monthly report locations also updated?

alright, some issues
that on adding location for ET, the site name is required
and if removing the site name from target location, the site name will still be with old name

actually i already fix this before, i comment the code that solve for that issue coz i think site_name defaultsTo "" will also fix that too and not to checki it again . But now it fix
fixbuget.mp4

updated for add location on monthly report location. do we keep this "site_name_checked" atrribute?

@drfaustusfade
Copy link
Owner Author

i would propose to have it for the future us, in case we will need checkbox

@drfaustusfade
Copy link
Owner Author

// on monthly add location validate, I think !l.site_name is missing, otherwise errors everywhere except ET
if (l.admin0pcode !== 'ET') {
					id = "label[for='ngm-new_location-site_name']";
					$(id).addClass('error');
					validation.divs.push(id);
					complete = false;
				}

@fakhrihawari
Copy link
Collaborator

// on monthly add location validate, I think !l.site_name is missing, otherwise errors everywhere except ET
if (l.admin0pcode !== 'ET') {
					id = "label[for='ngm-new_location-site_name']";
					$(id).addClass('error');
					validation.divs.push(id);
					complete = false;
				}

Thank you , updated

@drfaustusfade
Copy link
Owner Author

if (d.admin0pcode === 'NG') {
								rowComplete++;
							}else{
								if (d.site_name) {
									rowComplete++;
								}
							}

NG have confirmed not required site_name yet, in that case, I think site_name should be checked

@drfaustusfade
Copy link
Owner Author

Location names on monthly reports without site name end as next
, null
,
I think it is better visually not to have them

@fakhrihawari
Copy link
Collaborator

if (d.admin0pcode === 'NG') {
								rowComplete++;
							}else{
								if (d.site_name) {
									rowComplete++;
								}
							}

NG have confirmed not required site_name yet, in that case, I think site_name should be checked

i already updated that too , for now that site_name not be checked only for ET

@fakhrihawari
Copy link
Collaborator

Location names on monthly reports without site name end as next
, null
,
I think it is better visually not to have them

Updated, should we push?

@drfaustusfade
Copy link
Owner Author

looks alright, please recheck all functionality again where site_name is concerned and proceed with a pull request

@fakhrihawari
Copy link
Collaborator

okay

@drfaustusfade
Copy link
Owner Author

Notes:
markers look ok without site_name, i think no need to update

@fakhrihawari
Copy link
Collaborator

on PR

@drfaustusfade
Copy link
Owner Author

thank you, well done,
I was also thinking on forms, to have input next to checkbox like when site selection present with disabled input, or in place of checkbox to use selection from list, adding option to yes/no/ non-site

@fakhrihawari
Copy link
Collaborator

Screen Shot 2021-02-24 at 09 52 23

Screen Shot 2021-02-24 at 09 51 59

link branch updated
https://github.com/fakhrihawari/ngm-reportHub/tree/non-site_id-site_name

is like that?

@drfaustusfade
Copy link
Owner Author

drfaustusfade commented Feb 24, 2021

yes, could be, i think that is cleaner than the checkbox, by the cost of one more click, what do you think?

other design option, as you did, would be in place of Site ? and From List ? selections to have checkboxes with same site name input on the same line rather than separate, but that would be hard to put in balance to other elements

Note:
to change Site Name ? to label just Site ?

@fakhrihawari
Copy link
Collaborator

Yes, it more aligned using select rather than checkbox

@fakhrihawari
Copy link
Collaborator

fakhrihawari commented Feb 24, 2021

Screen Shot 2021-02-24 at 16 48 21

@drfaustusfade
Copy link
Owner Author

alright, lets have ? mark without space

@drfaustusfade
Copy link
Owner Author

NG confirmed not mandatory site name, please enable, but without site? selection

@fakhrihawari
Copy link
Collaborator

alright, lets have ? mark without space

updated

@fakhrihawari
Copy link
Collaborator

Link PR:
drfaustusfade/ngm-reportHub#113

@drfaustusfade
Copy link
Owner Author

alright, there seem to be some bugs, please test and compile test cases conducted
also please split NG enabling non-mandatory input, in a separate commit
location form yes/no site selection into a separate commit
and for now not enable site yes/no selection for ET,

I had
on adding location
chrome_6gUXG6ORgC

on changing site yes/no
chrome_11E1uhok8G

also for some reason next
chrome_xnT44EkB9N
after save
chrome_x2OxjWGu48

@fakhrihawari
Copy link
Collaborator

Miss that one , i will try solve this and push it as soon as posible,
and "now not enable site yes/no selection for ET" i think on master already like that

@fakhrihawari
Copy link
Collaborator

fakhrihawari commented Feb 26, 2021

Fix it like this

conduct_test.mp4

and sometime the site name not appear coz the type of site_id in location and in the list not same
like below
Screen Shot 2021-02-26 at 14 36 24

is just on my local all in prod too?

link : https://github.com/fakhrihawari/ngm-reportHub/tree/non-site_id-site_name

if you okay, i will make separate commit for NG

@fakhrihawari
Copy link
Collaborator

fakhrihawari commented Mar 1, 2021

NOTE:
Some admin2,admin3 code in AdminSite is different from list Admin2List and admin3list
in admin2list
Screen Shot 2021-03-01 at 10 18 09

in adminSIteList
Screen Shot 2021-03-01 at 10 17 53

So if we select site_name from list it become like this
Screen Shot 2021-03-01 at 10 19 45

@fakhrihawari
Copy link
Collaborator

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

No branches or pull requests

2 participants