[GT-187] Add feature selecting multiple sites#475
[GT-187] Add feature selecting multiple sites#475
Conversation
Sae126V
force-pushed
the
GT-187-(functionality)-Selecting-multiple-sites-in-a-downtime-view
branch
from
cfb67ca to
a87a37c
Compare
|
This PR is ready for review. |
| $type = strpos($idType, 's') !== false ? 'services' : 'endpoints'; | ||
| $id = str_replace(['s', 'e'], '', $idType); |
There was a problem hiding this comment.
can you add some comments here explaining the desired behaviour?
|
|
||
| if (!isset($_REQUEST['site_id']) || !is_numeric($_REQUEST['site_id']) ){ | ||
| throw new Exception("An id must be specified"); | ||
| if (empty($siteIDs)) { |
There was a problem hiding this comment.
is this change equivalent to the old functionality? what if a non numeric string is passed in via site_id?
There was a problem hiding this comment.
I would say Not an equivalent. We are now passing an array of Number. So I don't think it would be an issue.
There was a problem hiding this comment.
when you loop through the siteIDs later, can you add an is_numeric check?
|
When editing a downtime, the start time gets unset. This isn't existing beaviour. Can you track down the cause and fix? |
This would be fixed by #474 . |
Sae126V
force-pushed
the
GT-187-(functionality)-Selecting-multiple-sites-in-a-downtime-view
branch
from
1983632 to
b3dd024
Compare
| } | ||
| } | ||
|
|
||
| function hasSitesWithSingleTimezones() |
There was a problem hiding this comment.
this method should refer to selecting multiple sites, than sites being in the same timezone.
There was a problem hiding this comment.
I would like to go with hasSitesWithSingleTimezones() because its about user selecting site time zone (Which we are not supporting) option along with multiple sites.
There was a problem hiding this comment.
the check here is that multiple sites have been selected, not that wether those sites share a timezone.
There was a problem hiding this comment.
hasMultipleSitesWithSiteTimezones()
| $downtimeInfo['SITE_LEVEL_DETAILS'] = $siteLevelDetails; | ||
| $downtimeInfo['SERVICE_WITH_ENDPOINTS'] = $serviceWithEndpoints; |
There was a problem hiding this comment.
is SERVICE_WITH_ENDPOINTS roughly the same as the previous Impacted_Services here? if so, what does SITE_LEVEL_DETAILS add?
There was a problem hiding this comment.
is SERVICE_WITH_ENDPOINTS roughly the same as the previous Impacted_Services here?
No, SERVICE_WITH_ENDPOINTS will have each service with endpoints. Whereas Impacted_Services will have all service INFO.
I have used one object(SERVICE_WITH_ENDPOINTS ) for Viewing and extract the other to submit(Just in case if user goes back to and not submitting).
In a single loop. I constructed two objects one for viewing and the other for submitting purpose.
Sae126V
force-pushed
the
GT-187-(functionality)-Selecting-multiple-sites-in-a-downtime-view
branch
from
47a372c to
876e490
Compare
gregcorbett
left a comment
gregcorbett
left a comment
There was a problem hiding this comment.
I have left some additional comments
| foreach ($impactedIDs as $id) { | ||
| /** | ||
| * `$siteNumber` => It's about Site ID | ||
| * `$parentService` => It's about service ID endpoint belongs too | ||
| * `idType` => It's about to differentiate | ||
| * the endpoint vs service selection. | ||
| */ | ||
| list($siteNumber, $parentService, $idType) = explode(':', $id); | ||
|
|
||
| $type = strpos($idType, 's') !== false ? 'services' : 'endpoints'; |
There was a problem hiding this comment.
I'd go for something like:
| foreach ($impactedIDs as $id) { | |
| /** | |
| * `$siteNumber` => It's about Site ID | |
| * `$parentService` => It's about service ID endpoint belongs too | |
| * `idType` => It's about to differentiate | |
| * the endpoint vs service selection. | |
| */ | |
| list($siteNumber, $parentService, $idType) = explode(':', $id); | |
| $type = strpos($idType, 's') !== false ? 'services' : 'endpoints'; | |
| foreach ($impactedIDs as $id) { | |
| list($siteId, $parentServiceId, $idType) = explode(':', $id); | |
| // put typical input for $idType here. | |
| $type = strpos($idType, 's') !== false ? 'services' : 'endpoints'; |
|
@gregcorbett , I have updated the code (some what large parts). |
2 participants
[MAIN Functionality]
Added support for selecting multiple sites in "ADD" downtime.
Changed the Confirm Downtime and Confirm Edit Downtime pages.
Changed Add_Downtime success page with relevant content.
Resolves: #261
Partially address: #85 and #66 .
Addressed @gregcorbett comments:
Suggestion 1:
Include the Site Name next to the downtime link on creation, i.e.:
New Downtimes successfully created. Please click the links below for more information.
Site A: Downtime 10
Site B: Downtime 11
Suggestion 1:
Only display the start and end time once on the "Confirm Downtime" page, i.e.:
Severity: WARNING
Description: foo bar
Starting (UTC): 08/09/2023 00:00
Ending (UTC): 18/09/2023 00:00
Site Name: Brunnen GS
...
Site Name: Torch
...