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

feature: yes/no site_name (hidden) #119

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fakhrihawari
Copy link
Collaborator

No description provided.

update


update


update


fixbug target


update


update


update


update


updated


update
update


update


update
+ project.definition.admin0pcode
+ '&admin1pcode=' + location.admin1pcode
}).then(function (result) {
if (locationAdmin1pcodeTag !== location.admin1pcode) {
Copy link
Owner

Choose a reason for hiding this comment

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

that will make a request on each location, on the same admin1 that will return 304,
what will happen if locationAdmin1pcodeTag changes from A to B and then to A ( on refactor-location-handling locationAdmin1pcodeTag that is handled with array )
what if having a function on form widget that handles for locations unique admin sites requests if missing ( or whatever admin1,2,3 or site type by config, if taking into consideration configurable admin fetch service )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes i will change it like on refactor-location-handling,

Copy link
Owner

@drfaustusfade drfaustusfade left a comment

Choose a reason for hiding this comment

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

please check comment for setLocationAdminSelect
for hidden feature description please add more details and issue # for future reference

@fakhrihawari
Copy link
Collaborator Author

updated

@drfaustusfade
Copy link
Owner

updated

please try to handle adminSites requests in a separate function before setLocationAdminSelect, what happens if admin sites for admin1 already exist?

@fakhrihawari
Copy link
Collaborator Author

updated

please try to handle adminSites requests in a separate function before setLocationAdminSelect, what happens if admin sites for admin1 already exist?

So you want fetch all the adminsite first then running the setLocationAdminSelect?

@drfaustusfade
Copy link
Owner

drfaustusfade commented Mar 17, 2021

So you want fetch all the adminsite first then running the setLocationAdminSelect?

the idea is like that:
separately from setLocationAdminSelect there should be a function that checks if adminSites list contains the location admin ( if site_type and admin1 ( advanced based on config if admin2,3,4,5/site_type/cluster ), and requests if not

@fakhrihawari
Copy link
Collaborator Author

I put new function on separate function, so for now its just check if admin1 is on adminSites list or not , if not it will make request

@drfaustusfade
Copy link
Owner

I put new function on separate function, so for now its just check if admin1 is on adminSites list or not , if not it will make request

  1. I think setLocationAdminSelect should be used on the form widget and not hidden inside fetchInitialAdminSites, so that not to wait for admin site requests
  2. on fetchInitialAdminSites for each location check if site_type is present then proceed ( also maybe site_list_select_id )
  3. make requests when req_adminsites.length
  4. run setLocationAdminSelect if any/concat of the results have .length ( that will make request twice )
  5. consider adding stub function to handle configuration to handle different site requirements, for example in AF only Education schools would need to be fetched on admin2

with those notes let's comment/remove from this pull request and deal with that in a separate issue, we need to consider the details and test this functionality thoroughly as that will impact various operations,
additionally, the question of interest how that will behave with over 100 target locations

@fakhrihawari
Copy link
Collaborator Author

I put new function on separate function, so for now its just check if admin1 is on adminSites list or not , if not it will make request

  1. I think setLocationAdminSelect should be used on the form widget and not hidden inside fetchInitialAdminSites, so that not to wait for admin site requests
  2. on fetchInitialAdminSites for each location check if site_type is present then proceed ( also maybe site_list_select_id )
  3. make requests when req_adminsites.length
  4. run setLocationAdminSelect if any/concat of the results have .length ( that will make request twice )
  5. consider adding stub function to handle configuration to handle different site requirements, for example in AF only Education schools would need to be fetched on admin2

with those notes let's comment/remove from this pull request and deal with that in a separate issue, we need to consider the details and test this functionality thoroughly as that will impact various operations,
additionally, the question of interest how that will behave with over 100 target locations

but if setLocationAdminSelect not wait for adminsite request done the list adminsite foreach location is empty

@drfaustusfade
Copy link
Owner

yes, so that when complete, if adminsites updated setLocationAdminSelect will run once again ( better would be to update per location ), like that it is also not perfect, so that I propose to deal in a separate issue

@fakhrihawari
Copy link
Collaborator Author

fakhrihawari commented Mar 18, 2021

yes, so that when complete, if adminsites updated setLocationAdminSelect will run once again ( better would be to update per location ), like that it is also not perfect, so that I propose to deal in a separate issue

so you mean run setLocationAdminSelect itself and run again after fetchInitialAdminSites ?
and
no 5 right now the API adminsite only use admin1 not admin2

@drfaustusfade
Copy link
Owner

yes, so that when complete, if adminsites updated setLocationAdminSelect will run once again ( better would be to update per location ), like that it is also not perfect, so that I propose to deal in a separate issue

so you mean run setLocationAdminSelect itself and run again after fetchInitialAdminSites ?

yes, the idea to have fetchInitialAdminSites not dependant with setLocationAdminSelect, so that we need to elaborate on it

@fakhrihawari
Copy link
Collaborator Author

And no 5 on example you mean by fetched on admin2 ? is from API or just filter the list ?

@drfaustusfade
Copy link
Owner

that is to the idea of configuration for target locations
that is because of various requirements and list sized some operations need to request adminsites on login, some on admin1 change, some on admin2...

@fakhrihawari
Copy link
Collaborator Author

that is to the idea of configuration for target locations
that is because of various requirements and list sized some operations need to request adminsites on login, some on admin1 change, some on admin2...

okay i will procced this on other branch (refactor location handling)

update


update


update


update
@fakhrihawari
Copy link
Collaborator Author

yes, so that when complete, if adminsites updated setLocationAdminSelect will run once again ( better would be to update per location ), like that it is also not perfect, so that I propose to deal in a separate issue

so you mean run setLocationAdminSelect itself and run again after fetchInitialAdminSites ?

yes, the idea to have fetchInitialAdminSites not dependant with setLocationAdminSelect, so that we need to elaborate on it

And for this i already updated

@drfaustusfade
Copy link
Owner

on last pull's commit
when saving from list location, the location name disappears
i am not sure on this pull if yes/no should appear with empty list

chrome_a8QG3uH0Nk
chrome_3pAjSuVM0s

@fakhrihawari
Copy link
Collaborator Author

on last pull's commit
when saving from list location, the location name disappears
i am not sure on this pull if yes/no should appear with empty list

chrome_a8QG3uH0Nk
chrome_3pAjSuVM0s

from adminSites List
Screen Shot 2021-03-23 at 09 58 24

from target locations
Screen Shot 2021-03-23 at 09 58 38

type site_id from adminSites list is number but in the target location it saves as a string, and then when trying to match with site_id in adminsiteLists it doesn't match so that make location name disappear or not show

demononamesiteid.mp4

and for this "i am not sure on this pull if yes/no should appear with empty list " , can we just make it like this? if its empty list

Screen Shot 2021-03-23 at 11 25 43

@drfaustusfade
Copy link
Owner

  1. should be text
  2. if empty list there should not be selection

@fakhrihawari
Copy link
Collaborator Author

  1. should be text
  2. if empty list there should not be selection

updated

@drfaustusfade
Copy link
Owner

  • on no selection, if site type were selected before
    chrome_oSIOfhm8eQ

  • on admin3 change, from list displaying yes, after opening no

chrome_HYiiikBppx

chrome_0xcqRwqBiR

@fakhrihawari
Copy link
Collaborator Author

updated

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

Successfully merging this pull request may close these issues.

2 participants