futtta / ao_critcss_aas

Autoptimize power-up to integrate with criticalcss.com
9 stars 1 forks source link

feature idea: add URL to queue #22

Closed futtta closed 6 years ago

futtta commented 6 years ago

some (landing) pages might require their own CCSS which is different from a normal is_page() and currently the only way to ensure that right now is to create a manual rule.

so userstory:

As a ao_ccss_aas user I can add specific URL's to the queue, upon which the plugin automatically gets specific CCSS for them, ensuring they're (re-)fetched if needed (if the hash changes or ..., normal logic).

denydias commented 6 years ago

I didn't get this use case quite right... Isn't it exactly what manual rules are for?

Yes, manual rules blocks ccss.com fetch for a page. So, are we looking to a third rule case beyond auto and manual?

futtta commented 6 years ago

An auto rule based on a manual addition to the job queue :-)

we want to:

  1. allow the user to "force" a URL in the job queue as a path-based item
  2. CCSS is automatically created for that path
  3. this results in path-based rule with CCSS
  4. the CCSS can be regenerated if the page is re-entered in the queue automatically at a later point
denydias commented 6 years ago

So, as said before, we have a new rule class. ;)

This will require deep changes in the business rules as this use case wasn't forecast in specs nor implemented to handle it. As such, this is something for a major release (new feature). For now it belongs to backlog.

futtta commented 6 years ago

-> agree with changes in business rule -> agree with backlog

but to me this "looks" like a variation on the queue, not the rule class;

so changes to queue class, not to rule class? :-)

denydias commented 6 years ago

Both actually. Users should tell us that a manual rule should update its CCSS, which requires a new property in the rule object. Queue should read that property and act accordingly.

Anyway, this is deep. ;)

futtta commented 6 years ago

nope; as it's not a manual rule; the item was manually added to the queue, but it's fully automatic from that point onwards :-)

On Fri, Mar 23, 2018 at 9:17 AM, Deny Dias notifications@github.com wrote:

Both actually. Users should tell us that a manual rule should update its CCSS, which requires a new property in the rule object. Queue should read that property and act accordingly. Both things changes.

Anyway, this is deep. ;)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/22#issuecomment-375576365, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMdJf9Tv-DH5Egg6uqK5kVVeT6_vVks5thK-2gaJpZM4SyV_w .

denydias commented 6 years ago

Agreed. But still, a new property is required anyway, hence both rules and queue changes.

And another thing: just remember that a manual rules requires CSS content so it can be saved (the ajax service perform a check for this). This logic should be changed to handle that new 'manual auto' rule wit an empty CSS (but still require it for 'manual not auto' rules). Deeeeeepppp... :stuck_out_tongue_closed_eyes:

futtta commented 6 years ago

Deeeeeppp it is, but what new property do you see for rules?

On Fri, Mar 23, 2018 at 9:28 AM, Deny Dias notifications@github.com wrote:

Agreed. But still, a new property is required anyway, hence both rules and queue changes.

And another thing: just remember that a manual rules requires CSS content so it can be saved (the ajax service perform a check for this). This logic should be changed to handle that new 'manual auto' rule wit an empty CSS (but still require it for 'manual not auto' rules). Deeeeeepppp... 😝

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/22#issuecomment-375578548, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMc_tnY5iXNFE2682duAvFRhm0VpAks5thLI0gaJpZM4SyV_w .

denydias commented 6 years ago

but what new property do you see for rules?

fetch: false|true

Where:

This of course involves changes in the rule dialog too.

Do you want to display those rules with a different badge as MANUAL and AUTO we have today? If so, more UI changes ahead, including help text.

futtta commented 6 years ago

So I now (after playing around with the plugin some more) understand that currently we can add a MANUAL rule, leave the CCSS empty and it is (supposed to be) picked up at the next queue run;

image

The problem with this:

So I vote to remove "manual rules with auto-fetch" and instead have:

denydias commented 6 years ago

Man! I forgot we've changed that spec during development! I'm sorry.

Well, than what we need is to change that behavior so the rule with an empty content results in an AUTO one. A bit simpler, but still deep. Still, I'm glad we are half way there! ;)

futtta commented 6 years ago

change that behavior so the rule with an empty content results in an AUTO one

ideally it's not a rule, but an just an item manually added to the queue as type:page really. makes more sense, as a rule by definition has CCSS, which this one does not until it has been processed in the next queue run :)

denydias commented 6 years ago

Suppose the following rule object:

{
  "paths": {
    "/hello-world/": {
      "hash": 0,
      "file": "ccss_7e8672d3b6b62aab402abb31bbc01936.css"
    },
    "/2017/11/": {
      "hash": 0,
      "file": 0
    }
  },
  "types": {}
}

Both rules where created by the user. Path /hello-world has a CSS content entered in the textinput at rule creation dialog box, while path /2017/11/ has no CSS content entered (clear text box).

Job qualification goes as this:

2018-04-04T03:08:08+00:00 - [DD] Job submission DISQUALIFIED by MANUAL rule <paths|/hello-world/> with hash <0> and file <ccss_7e8672d3b6b62aab402abb31bbc01936.css>
2018-04-04T03:08:14+00:00 - [DD] Job submission QUALIFIED by AUTO rule <paths|/2017/11/> with hash <0> and file <0>
2018-04-04T03:08:14+00:00 - [DD] Job CREATED with local job id <j-u98bdl> for target rule <paths|/2017/11/>

At this point the queue object is:

{
  "ljid": "j-3ismi3",
  "rtarget": "paths|/2017/11/",
  "ptype": "is_archive",
  "hashes": [
    "fe4b0f6ebef3fe522a1038b01bdb04a5"
  ],
  "hash": "fe4b0f6ebef3fe522a1038b01bdb04a5",
  "file": null,
  "jid": null,
  "jqstat": "NEW",
  "jrstat": null,
  "jvstat": null,
  "jctime": 1522815593.1767,
  "jftime": null
}

When the queue run its first pass (generate):

2018-04-04T04:23:23+00:00 - [DD] Queue control started
2018-04-04T04:23:23+00:00 - [DD] Queue control locked
2018-04-04T04:23:23+00:00 - [DD] Processing job 1 of 1 with id <j-3ismi3> and status <NEW>
2018-04-04T04:23:23+00:00 - [DD] Found NEW job with local ID <j-3ismi3>, starting its queue processing
2018-04-04T04:23:23+00:00 - [DD] Job id <j-3ismi3> updated with SINGLE hash <fe4b0f6ebef3fe522a1038b01bdb04a5>
2018-04-04T04:23:23+00:00 - [DD] Job id <j-3ismi3> with hash <fe4b0f6ebef3fe522a1038b01bdb04a5> DOES NOT MATCH the one in rule <paths|/2017/11/>
2018-04-04T04:23:23+00:00 - [DD] criticalcss.com: POST generate request body is {"url":"http:\/\/aodev.ngrok.io\/2017\/11\/","aff":1}
2018-04-04T04:23:24+00:00 - [DD] criticalcss.com: POST generate request for path <http://aodev.ngrok.io/2017/11/> replied successfully
2018-04-04T04:23:24+00:00 - [DD] Job id <j-3ismi3> generate request successful, remote id <gen-6621579>, status now is <JOB_ONGOING>
2018-04-04T04:23:24+00:00 - [DD] Queue updated by job id <j-3ismi3>
2018-04-04T04:23:24+00:00 - [DD] Queue control unlocked
2018-04-04T04:23:24+00:00 - [DD] Queue control finished

And the second pass (results):

2018-04-04T04:25:12+00:00 - [DD] Queue control started
2018-04-04T04:25:12+00:00 - [DD] Queue control locked
2018-04-04T04:25:12+00:00 - [DD] Processing job 1 of 1 with id <j-3ismi3> and status <JOB_ONGOING>
2018-04-04T04:25:12+00:00 - [DD] Found PENDING job with local ID <j-3ismi3>, continuing its queue processing
2018-04-04T04:25:13+00:00 - [DD] criticalcss.com: GET results request for remote job id <gen-6621579> replied successfully
2018-04-04T04:25:13+00:00 - [DD] Critical CSS file for the rule <paths|/2017/11/> was saved as <ccss_0df79ac008a12123b575890333a5bdc2.css>, size in bytes is <10865>
2018-04-04T04:25:13+00:00 - [DD] Job id <j-3ismi3> result request successful, remote id <gen-6621579>, status <JOB_DONE>, file saved <ccss_0df79ac008a12123b575890333a5bdc2.css>
2018-04-04T04:25:13+00:00 - [DD] Queue updated by job id <j-3ismi3>
2018-04-04T04:25:13+00:00 - [DD] Target rule <paths|/2017/11/> of type <AUTO> was UPDATED for job id <j-3ismi3>
2018-04-04T04:25:13+00:00 - [DD] Job id <j-3ismi3> updated the target rule <paths|/2017/11/>
2018-04-04T04:25:13+00:00 - [DD] Queue control unlocked
2018-04-04T04:25:13+00:00 - [DD] Queue control finished

At the end we have the following rule object:

{
  "paths": {
    "/hello-world/": {
      "hash": 0,
      "file": "ccss_7e8672d3b6b62aab402abb31bbc01936.css"
    },
    "/2017/11/": {
      "hash": "fe4b0f6ebef3fe522a1038b01bdb04a5",
      "file": "ccss_0df79ac008a12123b575890333a5bdc2.css"
    }
  },
  "types": {}
}

Nobody can tell the rule paths|/2017/11/ was created by the user. From that point it behaves just like any other AUTO rule for that particular path.

All other behaviors were kept as is.

This is read to be tested.

futtta commented 6 years ago

explanation looks sweet Deny, I'll test this (and the other US) out later this week!

On Wed, Apr 4, 2018 at 6:49 AM, Deny Dias notifications@github.com wrote:

Suppose the following rule object:

{ "paths": { "/hello-world/": { "hash": 0, "file": "ccss_7e8672d3b6b62aab402abb31bbc01936.css" }, "/2017/11/": { "hash": 0, "file": "0" } }, "types": {} }

Both rules where created by the user. Path /hello-world has a CSS content entered in the textinput at rule creation dialog box, while path /2017/11/ has no CSS content entered (clear text box).

Job qualification goes as this:

2018-04-04T03:08:08+00:00 - [DD] Job submission DISQUALIFIED by MANUAL rule <paths|/hello-world/> with hash <0> and file 2018-04-04T03:08:14+00:00 - [DD] Job submission QUALIFIED by AUTO rule <paths|/2017/11/> with hash <0> and file <0> 2018-04-04T03:08:14+00:00 - [DD] Job CREATED with local job id for target rule <paths|/2017/11/>

At this point the queue object is:

{ "ljid": "j-3ismi3", "rtarget": "paths|/2017/11/", "ptype": "is_archive", "hashes": [ "fe4b0f6ebef3fe522a1038b01bdb04a5" ], "hash": "fe4b0f6ebef3fe522a1038b01bdb04a5", "file": null, "jid": null, "jqstat": "NEW", "jrstat": null, "jvstat": null, "jctime": 1522815593.1767, "jftime": null }

When the queue run its first pass (generate):

2018-04-04T04:23:23+00:00 - [DD] Queue control started 2018-04-04T04:23:23+00:00 - [DD] Queue control locked 2018-04-04T04:23:23+00:00 - [DD] Processing job 1 of 1 with id and status 2018-04-04T04:23:23+00:00 - [DD] Found NEW job with local ID , starting its queue processing 2018-04-04T04:23:23+00:00 - [DD] Job id updated with SINGLE hash 2018-04-04T04:23:23+00:00 - [DD] Job id with hash DOES NOT MATCH the one in rule <paths|/2017/11/> 2018-04-04T04:23:23+00:00 - [DD] criticalcss.com: POST generate request body is {"url":"http:\/\/aodev.ngrok.io\/2017\/11\/","aff":1} 2018-04-04T04:23:24+00:00 - [DD] criticalcss.com: POST generate request for path http://aodev.ngrok.io/2017/11/ replied successfully 2018-04-04T04:23:24+00:00 - [DD] Job id generate request successful, remote id , status now is 2018-04-04T04:23:24+00:00 - [DD] Queue updated by job id 2018-04-04T04:23:24+00:00 - [DD] Queue control unlocked 2018-04-04T04:23:24+00:00 - [DD] Queue control finished

And the second pass (results):

2018-04-04T04:25:12+00:00 - [DD] Queue control started 2018-04-04T04:25:12+00:00 - [DD] Queue control locked 2018-04-04T04:25:12+00:00 - [DD] Processing job 1 of 1 with id and status 2018-04-04T04:25:12+00:00 - [DD] Found PENDING job with local ID , continuing its queue processing 2018-04-04T04:25:13+00:00 - [DD] criticalcss.com: GET results request for remote job id replied successfully 2018-04-04T04:25:13+00:00 - [DD] Critical CSS file for the rule <paths|/2017/11/> was saved as , size in bytes is <10865> 2018-04-04T04:25:13+00:00 - [DD] Job id result request successful, remote id , status , file saved 2018-04-04T04:25:13+00:00 - [DD] Queue updated by job id 2018-04-04T04:25:13+00:00 - [DD] Target rule <paths|/2017/11/> of type was UPDATED for job id 2018-04-04T04:25:13+00:00 - [DD] Job id updated the target rule <paths|/2017/11/> 2018-04-04T04:25:13+00:00 - [DD] Queue control unlocked 2018-04-04T04:25:13+00:00 - [DD] Queue control finished

At the end we have the following rule object:

{ "paths": { "/hello-world/": { "hash": 0, "file": "ccss_7e8672d3b6b62aab402abb31bbc01936.css" }, "/2017/11/": { "hash": "fe4b0f6ebef3fe522a1038b01bdb04a5", "file": "ccss_0df79ac008a12123b575890333a5bdc2.css" } }, "types": {} }

Nobody can tell the rule paths|/2017/11/ was created by the user. From that point it behaves just like any other AUTO rule for that particular path.

All other behaviors were kept as is.

This is read to be tested.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/22#issuecomment-378479402, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMQ7CEsOloKcYLI0f5ZoIs03zgCnGks5tlFDygaJpZM4SyV_w .

futtta commented 6 years ago

Some confusion: I can now manually create a rule with a conditional tag and leave the CCSS empty, ao_ccss_aas accepts that and lists the rule as "To be fetched from criticalcss.com in the next queue run..." but that won't work, will it?

2 solutions:

So what do you think?

denydias commented 6 years ago

Some confusion: I can now manually create a rule...

No. What you can now is to manually add an AUTO rule IF you leave the CSS empty. This is what I understood from this.

...with a conditional tag and leave the CCSS empty, ao_ccss_aas accepts that and lists the rule as "To be fetched from criticalcss.com in the next queue run..." but that won't work, will it?

It should work just like paths. I haven't tested it for conditionals, but the logic behind is the same as paths, for which I've tested.

2 solutions:

A solution for what? I do not see a problem here.

  • change the placeholder-text if the user selects conditional tag + prevent manual rule with conditional tag from being submitted if the user does not enter CCSS
  • add an input field for "example URL" which can then be used by the job runner to automatically fetch the CCSS for that conditional tag

So what do you think?

I think: why do Frank wants to forbid users to manually create an AUTO rule for conditional tags if they need to do so?

If we just allow users to create AUTO rules for conditionals, the phrase...

To be fetched from criticalcss.com in the next queue run...

...sounds pretty good to me.

The manually created AUTO rule will fetch and hold the CSS for that path/conditional just like any other automatically created AUTO rule. The diference between then is that a user express the need to have a critical CSS for that particular path/conditional instead to rely just on or logic for that purpose.

Am I ludicrous wrong in those assumptions?

futtta commented 6 years ago

the reason why I think auto-generated rules form manually added rule with empty CCSS is that your job-runner requires a URL to feed to ccss.com. if you choose a conditional rule, you have a condition but not a URL, so you can't submit the rule with empty CCSS to ccss.com?

On Mon, Apr 9, 2018 at 10:06 PM, Deny Dias notifications@github.com wrote:

Some confusion: I can now manually create a rule...

No. What you can now is to manually add an AUTO rule IF you leave the CSS empty. This is what I understood from this http:///futtta/ao_critcss_aas/issues/22#issuecomment-374909819.

...with a conditional tag and leave the CCSS empty, ao_ccss_aas accepts that and lists the rule as "To be fetched from criticalcss.com in the next queue run..." but that won't work, will it?

It should work just like paths. I haven't tested it for conditionals, but the logic is the same as paths, for which I've tested.

2 solutions:

A solution for what? I do not see a problem here.

  • change the placeholder-text if the user selects conditional tag + prevent manual rule with conditional tag from being submitted if the user does not enter CCSS
  • add an input field for "example URL" which can then be used by the job runner to automatically fetch the CCSS for that conditional tag

So what do you think?

I think: why do Frank wants to forbid users to manually create an AUTO rule for conditional tags if they need to do so?

If we just allow users to create AUTO rules for conditionals, the phrase...

To be fetched from criticalcss.com in the next queue run...

...sounds pretty good to me.

The manually created AUTO rule will fetch and hold the CSS for that path/conditional just like any other automatically created AUTO rule. The diference between then is that a user express the need to have a critical CSS for that particular path/conditional instead to rely just on or logic for that purpose.

Am I ludicrous wrong in those assumptions?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/22#issuecomment-379877252, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMQ3ANqWkLxYPYwUqu_Lo4MWg4U0Nks5tm787gaJpZM4SyV_w .

denydias commented 6 years ago

Oh right! I'm ludicrous wrong indeed! :stuck_out_tongue_closed_eyes:

Automatically created AUTO rules for conditionals have a path associated to it. But manually created AUTO rules for conditionals don't.

In that case, I think the better is not to allow users to create a conditional rule with empty CSS. We can just validate that before save.

futtta commented 6 years ago

OK :-)

On Mon, Apr 9, 2018 at 10:32 PM, Deny Dias notifications@github.com wrote:

Oh right! I'm ludicrous wrong indeed! 😝

Automatically created AUTO rules for conditionals have a path associated to it. But manually created AUTO rules for conditionals don't.

In that case, I think the better is not to allow users to create a conditional rule with empty CSS. We can just validate that before save.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/futtta/ao_critcss_aas/issues/22#issuecomment-379884743, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMaWZD7mKLqBNVJ9yGsthWwMNKnjHks5tm8VegaJpZM4SyV_w .

denydias commented 6 years ago

New validation for types is ready for testing.

futtta commented 6 years ago

confirmed OK, thx Deny :)