Refactor site-related routes (#1668)

`/viewSite.php` has been redirected to `/sites/<id>` and `/siteStatistics.php` has been redirected to `/sites`. As noted in a comment in the routes file, it would be nice to change the behavior of `/sites` to show all sites the current user has access to, rather than just limiting the route to admin users.
Kitware · Aug 30, 2023 · 582d6ee · 582d6ee
1 parent 8c059e3
commit 582d6ee
Show file tree

Hide file tree

Showing 21 changed files with 34 additions and 37 deletions.
diff --git a/app/Http/Controllers/SiteController.php b/app/Http/Controllers/SiteController.php
@@ -372,20 +372,10 @@ public function editSite(): View|RedirectResponse
  ->with('xsl_content', generate_XSLT($xml, base_path() . '/app/cdash/public/editSite', true));
  }
 
- public function viewSite(): View
+ public function viewSite(int $siteid): View
  {
  $db = Database::getInstance();
 
- @$siteid = $_GET['siteid'];
- if ($siteid != null) {
- $siteid = pdo_real_escape_numeric($siteid);
- }
-
- // Checks
- if (!isset($siteid) || !is_numeric($siteid)) {
- abort(400, 'Not a valid siteid!');
- }
-
  $site_array = $db->executePreparedSingleRow("SELECT * FROM site WHERE id=?", [$siteid]);
  $sitename = $site_array['name'];
 

diff --git a/app/cdash/app/Controller/Api/QueryTests.php b/app/cdash/app/Controller/Api/QueryTests.php
@@ -415,7 +415,7 @@ public function getResponse()
 
  $test['labels'] = $row->labelstring;
 
- $siteLink = 'viewSite.php?siteid=' . $row->siteid;
+ $siteLink = 'sites/' . $row->siteid;
  $test['siteLink'] = $siteLink;
 
  $buildSummaryLink = "build/$buildid";

diff --git a/app/cdash/public/viewMap.xsl b/app/cdash/public/viewMap.xsl
@@ -58,7 +58,7 @@
 <tr>
 <td><center>
 <a>
-<xsl:attribute name="href">viewSite.php?siteid=<xsl:value-of select="id"/></xsl:attribute>
+<xsl:attribute name="href">sites/<xsl:value-of select="id"/></xsl:attribute>
 <xsl:value-of select="name"/></a></center></td>
 <td><center>
 <xsl:if test="string-length(maintainer_name)>1">

diff --git a/app/cdash/public/views/index.html b/app/cdash/public/views/index.html
@@ -70,7 +70,7 @@
  <br/>
  <div id="site" align="left">
  <b>Site</b>:
- <a ng-href="viewSite.php?siteid={{::cdash.siteid}}&project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">
+ <a ng-href="sites/{{::cdash.siteid}}?project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">
  {{::cdash.site}}
  </a>
  <img ng-if="::cdash.siteoutoforder == 1" src="img/flag.png" title="flag"/>
@@ -292,7 +292,7 @@ <h3>
  <tbody ng-if="::!cdash.coveragegroups || cdash.coveragegroups.length == 0" id="coveragebody">
  <tr class="child_row" ng-repeat="coverage in cdash.coverages |orderBy:sortCoverage.orderByFields" ng-class-odd="'odd'" ng-class-even="'even'">
  <td ng-if="::cdash.childview != 1" align="left" class="paddt">
- <a ng-href="viewSite.php?siteid={{::coverage.siteid}}&project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">
+ <a ng-href="sites/{{::coverage.siteid}}?project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">
  {{::coverage.site}}
  </a>
  </td>
@@ -413,7 +413,7 @@ <h3>
  </td>
 
  <td ng-if="::cdash.childview != 1" class="paddt" align="left">
- <a ng-href="viewSite.php?siteid={{::da.siteid}}&project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">
+ <a ng-href="sites/{{::da.siteid}}?project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">
  {{::da.site}}
  </a>
  </td>

diff --git a/app/cdash/public/views/partials/build.html b/app/cdash/public/views/partials/build.html
@@ -21,7 +21,7 @@
 
 <!-- Otherwise, show build name & site on the row. -->
 <td ng-if="::cdash.childview != 1" align="left" class="paddt">
- <a ng-href="viewSite.php?siteid={{::build.siteid}}&project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">{{::build.site}}</a>
+ <a ng-href="sites/{{::build.siteid}}?project={{::cdash.projectid}}&currenttime={{::cdash.unixtimestamp}}">{{::build.site}}</a>
  <img ng-if="::build.siteoutoforder == 1" border="0" src="img/flag.png" title="flag"></img>
 </td>
 

diff --git a/app/cdash/public/views/viewBuildError.html b/app/cdash/public/views/viewBuildError.html
@@ -2,7 +2,7 @@
  <tr>
  <td align="left">
  <b>Site: </b>
- <a href="viewSite.php?siteid={{cdash.build.siteid}}">
+ <a href="sites/{{cdash.build.siteid}}">
  {{cdash.build.site}}
  </a>
  </td>

diff --git a/app/cdash/tests/test_projectwebpage.php b/app/cdash/tests/test_projectwebpage.php
@@ -189,7 +189,7 @@ public function testProjectExperimentalLinkMachineName()
  $buildgroup = array_pop($jsonobj['buildgroups']);
  $siteid = $buildgroup['builds'][0]['siteid'];
 
- $content = $this->connect($this->url . "/viewSite.php?siteid=$siteid&project=4&currenttime=1235354400");
+ $content = $this->connect($this->url . "/sites/$siteid?project=4&currenttime=1235354400");
  if (!$content) {
  return;
  } elseif (!$this->findString($content, '<b>Total Physical Memory: </b>15MiB<br />')) {

diff --git a/app/cdash/tests/test_sitestatistics.php b/app/cdash/tests/test_sitestatistics.php
@@ -15,9 +15,9 @@ public function __construct()
  public function testSiteStatistics()
  {
  $this->login();
- $content = $this->get($this->url . '/siteStatistics.php');
+ $content = $this->get($this->url . '/sites');
  if (strpos($content, 'Busy time') === false) {
- $this->fail("'Busy time' not found on siteStatistics.php");
+ $this->fail("'Busy time' not found on /sites");
  }
  $this->pass('Passed');
  }

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -1288,7 +1288,7 @@ parameters:
  #^Call to deprecated function pdo_real_escape_numeric\\(\\)\\:
  04/01/2023$#
  """
- count: 5
+ count: 4
  path: app/Http/Controllers/SiteController.php
 
  -
@@ -1327,7 +1327,7 @@ parameters:
 
  -
  message: "#^Loose comparison via \"\\!\\=\" is not allowed\\.$#"
- count: 4
+ count: 3
  path: app/Http/Controllers/SiteController.php
 
  -

diff --git a/resources/js/components/BuildConfigure.vue b/resources/js/components/BuildConfigure.vue
@@ -15,7 +15,7 @@
  <tr>
  <td align="left">
  <b>Site: </b>
- <a :href="$baseURL + '/viewSite.php?siteid=' + cdash.build.siteid">
+ <a :href="$baseURL + '/sites/' + cdash.build.siteid">
  {{ cdash.build.site }}
  </a>
  </td>
@@ -105,7 +105,7 @@
  <tr>
  <td align="left">
  <b>Site: </b>
- <a :href="$baseURL + '/viewSite.php?siteid=' + cdash.build.siteid">
+ <a :href="$baseURL + '/sites/' + cdash.build.siteid">
  {{ cdash.build.site }}
  </a>
  </td>

diff --git a/resources/js/components/BuildNotes.vue b/resources/js/components/BuildNotes.vue
@@ -12,7 +12,7 @@
  <tr>
  <td align="left">
  <b>Site: </b>
- <a :href="$baseURL + '/viewSite.php?siteid=' + cdash.build.siteid">
+ <a :href="$baseURL + '/sites/' + cdash.build.siteid">
  {{ cdash.build.site }}
  </a>
  </td>

diff --git a/resources/js/components/BuildSummary.vue b/resources/js/components/BuildSummary.vue
@@ -36,7 +36,7 @@
  <td>
  <a
  id="site_link"
- :href="$baseURL + '/viewSite.php?siteid=' + cdash.build.siteid"
+ :href="$baseURL + '/sites/' + cdash.build.siteid"
  >
  {{ cdash.build.site }}
  </a>

diff --git a/resources/js/components/BuildUpdate.vue b/resources/js/components/BuildUpdate.vue
@@ -8,7 +8,7 @@
  </div>
  <div v-else>
  <h4 v-if="cdash.build.site">
- Files changed on <a :href="$baseURL + '/viewSite.php?siteid=' + cdash.build.siteid">{{ cdash.build.site }}</a>
+ Files changed on <a :href="$baseURL + '/sites/' + cdash.build.siteid">{{ cdash.build.site }}</a>
  ({{ cdash.build.buildname }}) as of {{ cdash.build.buildtime }}
  </h4>
 

diff --git a/resources/js/components/TestDetails.vue b/resources/js/components/TestDetails.vue
@@ -39,7 +39,7 @@
  </a>
  <a
  id="site_link"
- :href="$baseURL + '/viewSite.php?siteid=' + cdash.test.siteid"
+ :href="$baseURL + '/sites/' + cdash.test.siteid"
  >
  ({{ cdash.test.site }})
  </a>

diff --git a/resources/views/admin/user.blade.php b/resources/views/admin/user.blade.php
@@ -364,7 +364,7 @@
  </tr>
  <tr class="trodd">
  <td id="nob">
- <a href="siteStatistics.php">Site Statistics</a>
+ <a href="sites">Site Statistics</a>
  </td>
  </tr>
  <tr class="treven">

diff --git a/resources/views/site/site-statistics.blade.php b/resources/views/site/site-statistics.blade.php
@@ -15,7 +15,7 @@
  <tr>
  <td>
  <b>
- <a href="{{ url('viewSite.php') }}?siteid={{ $site->siteid }}">
+ <a href="{{ url('/sites/' . $site->siteid) }}">
  {{ $site->sitename }}
  </a>
  </b>

diff --git a/resources/views/test/view-test.blade.php b/resources/views/test/view-test.blade.php
@@ -25,7 +25,7 @@
  <b>Site Name:</b>
  </td>
  <td>
- <a ng-href="viewSite.php?siteid={{::cdash.build.siteid}}"
+ <a ng-href="sites/{{::cdash.build.siteid}}"
  ng-click="cancelAjax()">{{::cdash.build.site}}</a>
  </td>
  </tr>

diff --git a/routes/web.php b/routes/web.php
@@ -164,7 +164,11 @@
 // TODO: (williamjallen) Replace this /ajax route with an equivalent /api route
 Route::get('/ajax/getsubprojectdependencies.php', 'SubProjectController@ajaxDependenciesGraph');
 
-Route::get('/viewSite.php', 'SiteController@viewSite');
+Route::match(['get', 'post'], '/sites/{siteid}', 'SiteController@viewSite')->whereNumber('siteid');
+Route::get('/viewSite.php', function (Request $request) {
+ $siteid = $request->query('siteid');
+ return redirect("/sites/$siteid", 301);
+});
 
 Route::get('/viewMap.php', 'MapController@viewMap');
 
@@ -221,7 +225,10 @@
  Route::get('/removeBuilds.php', 'AdminController@removeBuilds');
  Route::post('/removeBuilds.php', 'AdminController@removeBuilds');
 
- Route::get('/siteStatistics.php', 'SiteController@siteStatistics');
+ // TODO: (williamjallen) Move this out of the admin-only section, and instead query only
+ // the sites a given user is able to see.
+ Route::get('/sites', 'SiteController@siteStatistics');
+ Route::permanentRedirect('/siteStatistics.php', '/sites');
 
  Route::get('/manageUsers.php', 'ManageUsersController@showPage');
  Route::post('/manageUsers.php', 'ManageUsersController@showPage');

diff --git a/tests/Feature/RouteAccessTest.php b/tests/Feature/RouteAccessTest.php
@@ -68,7 +68,7 @@ private function adminRoutes(): array
  return [
  ['/upgrade.php'],
  ['/removeBuilds.php'],
- ['/siteStatistics.php'],
+ ['/sites'],
  ['/manageUsers.php'],
  ['/monitor'],
  ['/ajax/findusers.php'],

diff --git a/tests/Spec/build-summary.spec.js b/tests/Spec/build-summary.spec.js
@@ -101,7 +101,7 @@ test('BuildSummary handles API response', async () => {
  expect(html).toContain('mysite');
  expect(html).toContain('Linux');
  const site_link = component.find('#site_link');
- expect(site_link.attributes('href')).toMatch('/viewSite.php?siteid=1');
+ expect(site_link.attributes('href')).toMatch('/sites/1');
  expect(site_link.text()).toBe('mysite');
 
  const configure_link = component.find('#configure_link');

diff --git a/tests/Spec/test-details.spec.js b/tests/Spec/test-details.spec.js
@@ -146,7 +146,7 @@ lines</pre>`);
 
  const site_link = component.find('#site_link');
  expect(site_link.text()).toBe('(my site)');
- expect(site_link.attributes('href')).toMatch('/viewSite.php?siteid=1');
+ expect(site_link.attributes('href')).toMatch('/sites/1');
 
  const revision_link = component.find('#revision_link');
  expect(revision_link.text()).toBe('asdf');