drfaustusfade / ngm-reportDesk

The workdesk for ReportHub
0 stars 1 forks source link

non site based reporting #58

Open drfaustusfade opened 3 years ago

drfaustusfade commented 3 years ago

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

drfaustusfade commented 3 years ago

to check if validation can be set per country

drfaustusfade commented 3 years ago

to check if titles without site_name will be displayed correctly

fakhrihawari commented 3 years ago

to check if titles without site_name will be displayed correctly

Screen Shot 2021-02-18 at 09 25 54

Just Like this?

fakhrihawari commented 3 years ago

to check if validation can be set per country

I think we can do that

fakhrihawari commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Mock up

https://user-images.githubusercontent.com/12655589/108474564-22eb4d00-72c2-11eb-9d71-6de449a222c4.mp4

drfaustusfade commented 3 years ago

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 commented 3 years ago

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

fakhrihawari commented 3 years ago

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

drfaustusfade commented 3 years ago

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

fakhrihawari commented 3 years ago

ok

fakhrihawari commented 3 years ago

Testing for ET https://github.com/fakhrihawari/ngm-reportHub/tree/non-site_id-site_name https://github.com/fakhrihawari/ngm-reportEngine/tree/non-site_id-site_name-be

Updated non site based old from too for NG

drfaustusfade commented 3 years ago

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

fakhrihawari commented 3 years ago

So for NG no need to change ?

drfaustusfade commented 3 years ago

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

fakhrihawari commented 3 years ago

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 commented 3 years ago

what is the reason?

fakhrihawari commented 3 years ago

what is the reason?

i dont know

drfaustusfade commented 3 years ago

alright, let's check, what are branches?

fakhrihawari commented 3 years ago

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

that one

fakhrihawari commented 3 years ago

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

fakhrihawari commented 3 years ago

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

drfaustusfade commented 3 years ago

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 commented 3 years ago

NG still have SiteName selection

fakhrihawari commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

https://user-images.githubusercontent.com/12655589/108792975-15c9a900-75b5-11eb-8815-a0c945a4b34c.mp4

drfaustusfade commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

drfaustusfade commented 3 years ago
// 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 commented 3 years ago
// 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 commented 3 years ago
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 commented 3 years ago

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

fakhrihawari commented 3 years ago
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 commented 3 years ago

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 commented 3 years ago

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

fakhrihawari commented 3 years ago

okay

drfaustusfade commented 3 years ago

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

fakhrihawari commented 3 years ago

on PR

drfaustusfade commented 3 years ago

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 commented 3 years ago
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 commented 3 years ago

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 commented 3 years ago

Yes, it more aligned using select rather than checkbox

fakhrihawari commented 3 years ago
Screen Shot 2021-02-24 at 16 48 21
drfaustusfade commented 3 years ago

alright, lets have ? mark without space

drfaustusfade commented 3 years ago

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

fakhrihawari commented 3 years ago

alright, lets have ? mark without space

updated

fakhrihawari commented 3 years ago

Link PR: https://github.com/drfaustusfade/ngm-reportHub/pull/113