I am puzzled by the error message: The migration class "Migration_Add_keywords_instances_field" is missing an "up" method."

I have the following settings in catalog/application/config/migration.php:

$config['migration_enabled'] = TRUE;
$config['migration_type'] = 'timestamp';
$config['migration_version'] = 20240516002600;
$config['migration_path'] = APPPATH . 'migrations/';

The following are the contents of catalog/application/migrations/20240516002600_add_keywords_instances_field.php

<?php

defined('BASEPATH') OR exit('No direct script access allowed');

class Migration_Add_keywords_instances_field extends CI_Migration {

        public function up()
        {
		$fields = array(
        		'instances' => array('type' => 'INT',
			'default' => '1',
			'after' => 'value')
		);
		$this->dbforge->add_column('keywords', $fields);
        }

        public function down()
        {
                $this->dbforge->drop_column('keywords', 'instances');
        }
}

I'm glad you're back to work on this! I think the "fix-previous" commits could stand to be squashed together, but this chain of commits is much more linear than the last. I'll let Artom request any changes he wants or needs on that front.

So, I've got some observations on what the code does, and I'll be happy to work with you on what might improve it on that front.

• Migration via php public_html/index.php migrate migrate went just fine! Tags appeared, all with instances at 1 until the cron job first runs. 👍
• I wasn't able to call the new cron endpoint (I tried curl -k https://librivox.org/cron/Keywords_stats/update_keywords_stats), until I added an 's' to the controller file's name, to match the class, "Keywords_stats.php".
  • After that, I could update the stats, and tags became click-able! The numbers also look right at first blush, though I'll go back and look for anomalies or off-by-ones at some point. 😉
• Clicking on a keyword took me straight to /keywords page, listing projects tagged with that keyword! 👍
  • It might be nice if this endpoint name was singular to match /author and /reader, but this is me nit-picking. 😝
  • This endpoint does not seem to reflect the sort order in the drop-down box. I think this comes down to get_projects_by_keywords_id_and_project_type in Project_model.php, where I see a sort order hard-coded.
  • When filtering this page by project type (solo vs group), the pagination links at the bottom still show the total number of pages. Judging by the other controllers, I think we're missing the project type constraint when defining $full_set.

Can I assume that where I make changes to address the issues you have raised, I should, as a matter of principle, ’squash’ the commit for each further change with the previous commit affecting the same area of code?

I believe that's the way to go - each commit in the final PR should result in working code, even if it isn't used yet. Beyond that, it seems quite subjective, though perhaps I'm missing a convenient guideline.

Myself, I'd put the PHP 8 fix and the configuration change all into the first commit, so that it results in working code that completes one "thing". All the future commits can move on from that to other "things", without changing the code from your first commit unless it's really necessary.
This does call for rewriting history (after all, we didn't know the migration had a PHP 8 problem until chronologically late!), but Artom mentioned some tools for helping with that.

I have had a go at addressing what I think are the major concerns redrun has expressed so far (though I'm afraid I don't understand redrun's point about the cron job endpoint, and have not addressed that). I have now had a (reasonably brutal!) first person introduction to rewriting a git commit history — the need for such, the means at one's disposal, and the many (many!) pitfalls lying in wait for the beginner. I hope I've made some useful progress in this area too.

This looks (to my eye) much, much better! I wanted to send you to a straightforward tutorial and explanation, but the stuff I found mostly made my head hurt, even though I assimilated some of the 'how' of things. I'm glad you also "made it through". 😉

My cron job issue comes down to a test: set the instances counts to something obviously incorrect (say, 1) , and try to trigger the stats update (curl -k https://librivox.org/cron/Keywords_stats/update_keywords_stats is how I tried triggering it). What I saw was HTML in the terminal output, and the instances counts did not update. Is that what you see?

I fixed it by changing the name of the file 'application/controllers/cron/Keyword_stats.php'. Since the class it implements is called 'Keywords_stats' (note the plural), I added an 's' to the filename, and then it worked.

When it comes to updating a commit when there are later ones in the chain, I believe either git rebase or git cherry-pick could save you from needing to re-do the later commits. I'd have to (save several backup copies, and then) play around to find the right sequence of commands and edits.

Thanks for that explanation of the cron job issue (which should have been staring me in the face). I've fixed that now, and have pushed that change and all "subsequent" changes here again.

Overall, this looks to be working well enough, that I can really take a hammer to it next time I get the chance! 😆
These are all the notes I've got for now, and it's fine by me if you wait for that additional testing before acting on them.

Well, I've finally sat down to configure my dev box to catalog mock projects again. I need to do a PR to add this to the Ansible scripts sometime. 🤔

That was the last test I knew we couldn't do on the staging server. If you'd like to implement the changes I previously mentioned, I won't hold you up any longer, and it can go to staging.

For convenience, here are quick summaries of those three things:

• Technical - the .gitignore file being removed from the repo
• Technical - Project_keyword_model.php line 19 concatenating the project id, instead of using a '?'. (Line 22 sets up the DB library so that it will escape and substitute the id in place of the '?', which is the preferred method.)
• Usability suggestion - in views/catalog/keywords.php, add a header to display the particular keyword we're browsing.

Once at least the first two are addressed, we'll ask if Artom can load it up on Staging for more people to play with. Perhaps along with #231, for the full experience! 😉

Then on a more personal note, I know that text isn't my best medium for building empathy. Code reviews, in particular, seem very "transactional" to me, as they're all focused on business. They'd be harder to read, if they weren't!
But that can make them, if not antagonistic, at least a little adversarial, which I know I find frustrating from the relationship side.

I want to say: Thank you!
Thank you for caring in the first place to suggest alleviations for the pain-points you see.
Thank you for being respectful as you made those suggestions, rather than expecting somebody to "just" do X.

In our forum conversation, I was most explicit in saying that I would not ask someone to work on our code, for reasons that extend beyond the messy code itself. But I'm glad you did. Thank you again for volunteering - and learning, and returning - to work on this anyway.

To make this happen, we needed you. With the amount of new code this took, and the number of other bugs and quirks that have bothered volunteers for years, I don't know if I'd have ever gotten to such a big new feature. Between you and Gareth (and hopefully me), we've made and are making these changes in the past few months, that folks will enjoy for years, even if nobody specifically asked for them to happen right now.

You and I are excited to work on the code, as and when we can. Please be patient with me, as I attempt to save other volunteers (who did not sign up to do code review or QA testing) as much work, worry and stress as possible. Significant updates come with all of those things, for more than just us folks here on GitHub. Even a small update can have unexpected quirks, and we don't want to pressure people to find them before they have to live with them. (See also: #233 😓 )
Thank you for your patience so far, and I hope we'll all get to see this live fairly soon!

Well, I've finally sat down to configure my dev box to catalog mock projects again. I need to do a PR to add this to the Ansible scripts sometime. 🤔

That was the last test I knew we couldn't do on the staging server. If you'd like to implement the changes I previously mentioned, I won't hold you up any longer, and it can go to staging.

For convenience, here are quick summaries of those three things:

• Technical - the .gitignore file being removed from the repo
• Technical - Project_keyword_model.php line 19 concatenating the project id, instead of using a '?'. (Line 22 sets up the DB library so that it will escape and substitute the id in place of the '?', which is the preferred method.)
• Usability suggestion - in views/catalog/keywords.php, add a header to display the particular keyword we're browsing.

Once at least the first two are addressed, we'll ask if Artom can load it up on Staging for more people to play with. Perhaps along with #231, for the full experience! 😉

Then on a more personal note, I know that text isn't my best medium for building empathy. Code reviews, in particular, seem very "transactional" to me, as they're all focused on business. They'd be harder to read, if they weren't! But that can make them, if not antagonistic, at least a little adversarial, which I know I find frustrating from the relationship side.

I want to say: Thank you! Thank you for caring in the first place to suggest alleviations for the pain-points you see. Thank you for being respectful as you made those suggestions, rather than expecting somebody to "just" do X.

In our forum conversation, I was most explicit in saying that I would not ask someone to work on our code, for reasons that extend beyond the messy code itself. But I'm glad you did. Thank you again for volunteering - and learning, and returning - to work on this anyway.

To make this happen, we needed you. With the amount of new code this took, and the number of other bugs and quirks that have bothered volunteers for years, I don't know if I'd have ever gotten to such a big new feature. Between you and Gareth (and hopefully me), we've made and are making these changes in the past few months, that folks will enjoy for years, even if nobody specifically asked for them to happen right now.

You and I are excited to work on the code, as and when we can. Please be patient with me, as I attempt to save other volunteers (who did not sign up to do code review or QA testing) as much work, worry and stress as possible. Significant updates come with all of those things, for more than just us folks here on GitHub. Even a small update can have unexpected quirks, and we don't want to pressure people to find them before they have to live with them. (See also: #233 😓 ) Thank you for your patience so far, and I hope we'll all get to see this live fairly soon!

I have now attempted to address all three of the technical issues you have (very conveniently - thank you!) summarised here and have pushed the changes through.

On the personal side, I doubt if you can have any idea how glad and thankful I was to receive your very kind, generous and timely message. You have been the soul of discretion, and the soul of helpfulness, through this whole process, for ever acting in what I have long since come to recognise as the true Librivox spirit. If I can live up to that ethos half as well as you do, I will be very glad indeed (😃) - and probably a bit surprised, too!

Excellent, I'm glad my nit-picking hasn't driven you away just yet!
So far as I'm concerned, the LibriVox spirit is: "volunteers, helping volunteers." I know you've got the same spirit in mind, and I just hope for a reminder whenever I get lost in the weeds myself. 😄

Back to the business side, those changes address everything I know we need for this PR. You mentioned you might make changes to the other one, but I think we can call this one "Ready for Staging"!

• In terms of what it does: Looks good to me, but wiser heads than mine should do some further testing.
• In terms of code: Similarly, if Gareth, Artom or anyone has further suggestions... 😉

Excellent, I'm glad my nit-picking hasn't driven you away just yet! So far as I'm concerned, the LibriVox spirit is: "volunteers, helping volunteers." I know you've got the same spirit in mind, and I just hope for a reminder whenever I get lost in the weeds myself. 😄

Back to the business side, those changes address everything I know we need for this PR. You mentioned you might make changes to the other one, but I think we can call this one "Ready for Staging"!

• In terms of what it does: Looks good to me, but wiser heads than mine should do some further testing.
• In terms of code: Similarly, if Gareth, Artom or anyone has further suggestions... 😉

Thank you. I'll try to steel myself for whatever may follow 😄! As for "the other one", l have already made further changes to #231 which I'm personally happy with. Those changes, if acceptable for staging yet, might complete the changes here nicely.

It's possible that when administrators see this capability in action, they'll be quietly (or not so quietly) horrified at some of the keywords they see are in actual use. It occurs to me that one way to alleviate this horror ever so slightly might be to provide a function that made it easy for administrators quickly to edit those keywords. While it's not necessarily ideal for this purpose, the "add_catalog_item" page does support this functionality, and it's a relatively simple exercise to add to the project catalog page a link, visible only to logged in administrators, that would take them straight to this page for the project concerned. I've mocked that up on my local development system as a proof-of-concept. Hopefully the attached document illustrates properly what I mean.
Support easy editing of keywords for existing projects.pdf

This link looks like a nice convenience feature in general! It's much more useful for other things than for keywords, though. I did say that updating keywords is far beyond the scope of a PR. My hope all along is that this can be genuinely useful without that. 😅

I hope I haven't created a false impression that updating keywords would be a common thing: in fact the keywords containing ";" are an exceptionally easy case because they only involve the "add_catalog_item" page. I could go into a little bit more detail in a PM some other time. So far, I haven't seen any "can't you just" here on GH, but some folks may come along who don't yet know better.

Perhaps this new link should be its own thing, over in the side-bar. And though I'm sorry to add more for Artom to sort through, I think a separate PR would be appropriate.

This link looks like a nice convenience feature in general! It's much more useful for other things than for keywords, though.
...
Perhaps this new link should be its own thing, over in the side-bar. And though I'm sorry to add more for Artom to sort through, I think a separate PR would be appropriate.

Good idea - but sorry Artom! Have put this forward now as #234

I attempted two pushes that address notartom's review comments so far around 14 hours ago. The changes I have made have not, as far as I can see from a quick check, "broken" anything functionally on my local development system, certainly not with respect to the new keywords displaying capability. The first push/deployment failed because I had left some glitchy code stuff in Migration.php. After I cleaned that up, I tried again. The second push/deployment has failed at a website health check. I'm not sure I know how to troubleshoot that from where I'm sitting. Would appreciate advice on how best to proceed from here.

The second push/deployment has failed at a website health check. I'm not sure I know how to troubleshoot that from where I'm sitting. Would appreciate advice on how best to proceed from here.

I think I see the trouble: this first commit includes the instruction files for running the two new migrations, but it does not set $config['migration_version'] = 3 in application/config/migration.php, which would tell CodeIgniter to run those migrations. This causes the Page controller to crash when it tries to use the new count column, which hasn't been created.

If you can also remove the change to system/libraries/Migration.php from the same commit, I think the migration is all set without that line. 😉

The second push/deployment has failed at a website health check. I'm not sure I know how to troubleshoot that from where I'm sitting. Would appreciate advice on how best to proceed from here.

I think I see the trouble: this first commit includes the instruction files for running the two new migrations, but it does not set $config['migration_version'] = 3 in application/config/migration.php, which would tell CodeIgniter to run those migrations. This causes the Page controller to crash when it tries to use the new count column, which hasn't been created.

If you can also remove the change to system/libraries/Migration.php from the same commit, I think the migration is all set without that line. 😉

Thanks for your advice, which I've followed as best I can. Deployment, at least, seems to have succeeded now.

Excellent!

Oh and... woops. Sorry Artom, I didn't mean do "request a review", I know you'll see this in your own time.
I checked this on mobile, and I was expecting a tool-tip or something to tell me exactly what that little +/- icon meant. 😬

It concerns me that on the list of Pull requests (https://github.com/LibriVox/librivox-catalog/pulls) this is still showing up as "Changes requested", which seems to imply that the code has been at least partly reviewed, and I have not followed up by making requested changes. This is not the case. When changes were requested more than two months ago, I addressed them pretty promptly — and have heard nothing since.

@peterjdann Honestly, I'm not sure how to change that. All the technical changes we've requested, I believe you're up to date with. The non-technical questions are... well, moving slowly, and I hope to have some news for you on that soon-ish, but don't let the GitHub status bother you if you can help it. 😅

@peterjdann - It sure has been an interesting couple of months. Now that the dust is mostly settled from the Archive incident, we are looking at this again, and I think concerns are addressed to where it can go live in a far more soonish soon.

Of course, there had to be one more thing. 😨
I sent in #247 to update some URLs in our database all at once. And since that was a migration, and I took migration #2, these will need to be 3 and 4, and config/migration.php updated in tandem.
If you'd like, I could try a git rebase on this myself. Up to you.

@peterjdann - It sure has been an interesting couple of months. Now that the dust is mostly settled from the Archive incident, we are looking at this again, and I think concerns are addressed to where it can go live in a far more soonish soon.

Of course, there had to be one more thing. 😨 I sent in #247 to update some URLs in our database all at once. And since that was a migration, and I took migration #2, these will need to be 3 and 4, and config/migration.php updated in tandem. If you'd like, I could try a git rebase on this myself. Up to you.

I have had a go at addressing this. I think I understand what's involved, and have pushed through what I think is needed. Please do let me know if I've got the wrong end of the stick!

Looks like the right end to me! The end result gets us right up to date with master. Only thing I see is that the migration version is now at 3, so the (newly re-numbered) number 4 won't be called. I might also suggest keeping the two migrations in separate commits - one of them is not "undoable", but the other is.

Looks like the right end to me! The end result gets us right up to date with master. Only thing I see is that the migration version is now at 3, so the (newly re-numbered) number 4 won't be called. I might also suggest keeping the two migrations in separate commits - one of them is not "undoable", but the other is.

Well, I had part of the stick! Have now put the two migration actions in separate commits, and have created a separate migration version setting for each ("3" then "4").

OK! So, the non-technical side is all caught up. So far as admins are concerned, this can go live as soon as we're ready on this side. 👍

On this end of things, I hope you'll forgive me. I've been learning a LOT lately, and some things about the code itself are starting to click. So, sitting down for a final review with what I know now, I have some additional technical comments. Please don't hate me. 😓
I've started a review, but I think you'll only be able to see it once I finish and post it.