Refactor content/logic files into subclasses of Page or Action #132

Closed
Krinkle opened this Issue Mar 24, 2012 · 3 comments

Comments

Projects
None yet
2 participants
Member

Krinkle commented Mar 24, 2012

Right now the url structure is not consistent across the various actions (or "states" as they're currently called). And although the logic and inde.php html response content are separated, it is not very re-usable for e.g. an API (issue #115).

Plan:

  • Normalize url structure
    • One parameter name for determining which logic to use and one parameter name to use as argument for that logic. The current .htaccess-sample file demonstrates the complexity due to the need for a separate rewrite rule for each action and a naming mismatch between the virtual name and actual state logic (job vs jobstatus, user vs tinder) (run/(.*) -> state=run&user=$1, login/ -> state=login, user/(.*) -> state=tinder&user=$1, job/(.*) -> state=jobstatus&job_id=$1, etc.)
    • Proposing to use something simple like action=$1&item=$2, where item is optional (the action should now what to do if there is no item given and what to do if there is an invalid item given).
  • Implement Action classes
  • Implement Page classes

Krinkle was assigned Mar 24, 2012

@jzaefferer jzaefferer added a commit that referenced this issue Mar 31, 2012

@jzaefferer jzaefferer [issue #132] Convert signup page and action to new system. Kick out e…
…mail and request fields, don't need them anymore
549ac3f
Owner

jzaefferer commented Mar 31, 2012

addjob and the three wipe actions remain.

JobPage and UserPage both have two get_status functions inline, and probably other duplication, those need to be cleaned up. I didn't know where to put that code though.

@Krinkle Krinkle added a commit that referenced this issue Mar 31, 2012

@Krinkle Krinkle [issue #132] Convert action=wipe into Action/Page class, renamed to "…
…cleanup"

- Just like a few others, this should be an Api only, so we use the same
  page-wrapper with try-catch as GetrunPage etc.

- Remove cronjob.txt

- Update README
076edf3
Member

Krinkle commented Mar 31, 2012

Thanks a lot for all those conversions!

I'm currently implementing JobAction for #116, I think I'll convert those duplicate local functions you mentions as pubic static methods in the JobAction class.

Krinkle closed this Apr 1, 2012

@Krinkle Krinkle added a commit that referenced this issue Apr 1, 2012

@Krinkle Krinkle Merge #132, #115 into 'master' 2d05540
Owner

jzaefferer commented Apr 1, 2012

I think I reviewed all new commits. Great work! I'll do some more testing tomorrow.

@Krinkle Krinkle added a commit that referenced this issue Apr 1, 2012

@Krinkle Krinkle [issue #132 / #115] Convert wipe job to WipejobAction
- Adding error code "require-auth"

- Changing window.SWARM to be like InfoAction instead of an ad-hoc thing
  in Page::output.

- Fix an E_NOTICE in inc/pages/JobPage.php
  Undefined index $data["jobInfo"]

- Since wipejob is no longer a page, now using the API through AJAX instead
  of a <form>. And instead of refreshing, just triggering the
  refreshTable() earlier (in case of type=reset) or redirecting to
  the user page (in case of type=delete).

  Also doing proper error handling now (previously the POST would
  suppress  any errors and just redirect unconditionally).
b5c783e

@Krinkle Krinkle added a commit that referenced this issue Apr 2, 2012

@Krinkle Krinkle [issue #132 / #115] Convert addjob to Page/Action classes
- AddjobAction:
 * Renamed the parameters to be more descriptive.
 * Fix various minor bugs (such as inserting empty runs), caused by
   submitting the <form> with more "runNames[]" inputs then filled in.
   Filtering out empty-string submissions.

- AddjobPage:
 * Removed superfluous <legend> wrappers around each input/label
 * Added short description to the runs section, example for QUnit
   (every QUnit test suite module mapping to a run)
 * Using placeholder="http://" instead of value="http://"
 * Adding for="" attributes
 * Adding a bit of JS to add more run entries.

- Removed 'status' column of 'runs' table (for the same reason
  the 'status' column of the 'jobs' table was previously deleted.
  It is not used anywhere and only some code paths keep it up to date.

  We could do this type of aggregation, but if we do so we'll have to keep
  it really good up to date otherwise it is useless.

  Right now I've opted to remove it since it wasn't used. All actions that
  request status of runs or jobs query the related tables and summarize the
  information on the fly.

  (removed query from wiprun.php, WipejobAction.php and *.sql)

- WipejobAction:
 * Fixed a bug: Jobs with 0 runs couldn't be deleted because the DELETE
   query only ran inside the if-statement for runRows.
d39e8f0

@Krinkle Krinkle added a commit that referenced this issue Apr 2, 2012

@Krinkle Krinkle [issue #132 / #115] Convert wiperun to Action class
- Instead of (abu)using the runresults <a href>, exporting them
  via data- attributes.

- Converted wiperun to WiperunAction
  Now also requires job_id, useragent_id to avoid deleting
  something else in a race condition

- Using the new api.php action=wiperun in job.js
  Checking "runStatus !== new" instead of "has(a)"
cec1df6

@Krinkle Krinkle added a commit that referenced this issue Apr 2, 2012

@Krinkle Krinkle [issue #132] clean up old raw action/pages includes
- All actions/pages have been converted :)
a1b3a2e

@Krinkle Krinkle added a commit that referenced this issue Apr 6, 2012

@Krinkle Krinkle [issue #143] Convert UserPage to new user agent system + Implement Fa…
…uxRequestContext

- Extracted the logic into UserAction (issue #132)

- Dropped support for the ?job= parameter that functioned as a primitive
  search filter for certain job names, but was kinda useless since
  it stripped out characters commonly found in job names (such as numbers
  and full-stops) and even then, job names are sorted by creation,
  and then mysql_queryf escapes some parts.

  Could be easily (re)implemented "the right way" over in the UserAction.
  Anyone interested may create an issue for it.

- Converted BrowserInfo::getBrowscap() into an array with a known set of
  properties instead of blindly passing the original object which has many
  other properties that should not be used.

- DerivativeWebRequest, Context::createDerivedRequestContext
 * New class that takes query parameters and POST boolean
   and creates a fake Context object derived from the given
   context object. Used to use JobAction inside UserAction.

- Fixed #13 (again). This made it visible that status-new should be grey
  and status-notscheduled white (which is how it was before twitter bootstrap)

- Added json_decode/json_encode normalization to Action::setData.
  This way all Action responses contain associative arrays and never
  objects.

- Poke #137, using <div class="alert alert-error"> for error messages.
28b6595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment