FoF Terms, ask your users to accept TOS and Privacy Policy

luceos

Pollux inside the migrations table delete any entries related to the extension, otherwise the migrations won't re-run.

Pollux

luceos This didn't help.

Reinstalling terms doesn't perform any changes to the database anymore. It doesn't create any of the new tables nor does it create new entries in the migrations or settings table..

luceos

Pollux re-installing does not run migrations, enabling does. So you would have to re-enable it if you uninstalled it when enabled after removing the migrations entries and tables related to the extension (does this even make sense ? )

Pollux

luceos does this even make sense ?

Halfway ?

But I'm one step further. My problem was, that I could not access the Admin panel because of the error described above (Table 'flarum.flagrow_terms_policies' doesn't exist). I therefore assumed, that the migration took place during installation and not after enabling it.

So how could the Admin panel complain if the extension was a fresh install?

The answer: settings.extensions_enabled still contained your extension because obviously I deinstalled it while still enabled. So after re-installing it it was automatically viewed as enabled without running the migration.

So, now I have terms installed and the Admin panel doesn't crash immediately anymore. Now I try to enable the extension:

exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1215 Cannot add foreign key constraint' in /path/to/flarum/vendor/illuminate/database/Connection.php:390\nStack trace:\n#0 /path/to/flarum/vendor/illuminate/database/Connection.php(390): PDOStatement->execute(Array)\n#1 /path/to/flarum/vendor/illuminate/database/Connection.php(655): Illuminate\\Database\\Connection->Illuminate\\Database\\{closure}(Object(Illuminate\\Database\\MySqlConnection), 'alter table `fl...', Array) ...

Like before, flagrow_terms_policies and flagrow_terms_policy_user are created as InnoDB tables. I don't know whether that's the reason why the foreign key constraint can't be added.

Shall I try the path of changing the default storage engine to MyISAM again?

Pollux

I just queried my database: SHOW ENGINE INNODB STATUS

The result:

------------------------
LATEST FOREIGN KEY ERROR
------------------------
2018-05-09 13:05:35 0x7000044e1000 Error in foreign key constraint of table flarum/#sql-11ba_5c:
foreign key (`user_id`) references `users` (`id`) on delete cascade:
Cannot resolve table name close to:
(`id`) on delete cascade

Does that help?

Edit: As I understand the engine should be the same in both tables. But users is MyISAM and flagrow_terms_policy_user is InnoDB. I guess that's causing the error.

luceos

Pollux once you deleted the tables manually, you should also have deleted the foreign key constraints added by the migrations. Now that you didn't the migrations fail. You will have to remove the foreign key constraints from your database as well before trying to re-enable the extension.

This is one of the reasons why doing these things manually often is a bad idea ?

clarkwinkelmann

Wow there's a lot discussed there.

Pollux Edit: As I understand the engine should be the same in both tables. But users is MyISAM and flagrow_terms_policy_user is InnoDB. I guess that's causing the error.

Only the posts table should be MyISAM in a normal Flarum install. Indeed the Terms migrations expect users to be InnoDB, otherwise MySQL won't be able to create a foreign key (foreign keys are an InnoDB thing I believe).

You could skip adding that foreign key if creating the tables manually, this won't cause any serious issue. There might just be dead records in the table flagrow_terms_policiy_user after a while if you delete users.

luceos The down method should have deleted the foreign key constraint, which is possible using dropForeign

When adding a column and a foreign key as part of a migration, dropForeign should indeed be used in the down part. But I've never used it when dropping a table altogether. Dropping a table drops the foreign keys automatically. If users had a foreign key contraint referencing the table we're deleting that would be another story, we'd need to first drop all foreign contraints targeting the table we're deleting. But it's rare as you usually run migrations down in a logicial order (inverse of creation).

Pollux This foreign key needs to be removed, the column can't simply be dropped.

Correct me if I'm wrong, but as I pointed above we're deleting the whole table so both column and keys automatically go away normally.

Pollux

luceos This is one of the reasons why doing these things manually often is a bad idea ?

That's what I'm learning right now. ? But that's OK for me, I like learning. ?

luceos once you deleted the tables manually, you should also have deleted the foreign key constraints added by the migrations.

So how do I delete the foreign key constraints in MySQL? The only hack I found was making a copy of the users table without constraints and replace users with that otherwise identical copy.

But this didn't solve my problem. I still get the General error: 1215 Cannot add foreign key constraint

Are you sure, that the different storage engines aren't a problem? Or should I try to change the default storage engine to MyISAM? What storage engines are supposed to be used in the users table and in those of your extension?

Pollux

clarkwinkelmann Only the posts table should be MyISAM in a normal Flarum install.

That's interesting. In my two installs (one locally and one on shared hosting) both via the standard composer way, all tables are MyISAM except the two new tables of the terms extension.

clarkwinkelmann Indeed the Terms migrations expect users to be InnoDB, otherwise MySQL won't be able to create a foreign key (foreign keys are an InnoDB thing I believe).

That's what I have learnt in the last hours. So mixing both storage engines doesn't seem a problem.

And if all my other tables are MyISAM, that means that foreign key constraints aren't used within the rest of Flarum?

clarkwinkelmann Dropping a table drops the foreign keys automatically.

So the foreign keys are somehow attached to the child table? In the case of:

$table->foreign('user_id')->references('id')->on('users')->onDelete('cascade');

... the foreign keys are attached to the flagrow_terms_policy_user table and not the users table?

But why did you, Luceos, write:

luceos once you deleted the tables manually, you should also have deleted the foreign key constraints added by the migrations. Now that you didn't the migrations fail.

Are there different ways of deleting a table? if I use the drop table button in phpMyAdmin to drop flagrow_terms_policy_user, doesn't that delete the foreign key constraints as well? Are there any chances that foreign key constraints survive such a drop?

So coming backt to the InnoDB status error:

------------------------
LATEST FOREIGN KEY ERROR
------------------------
2018-05-09 13:05:35 0x7000044e1000 Error in foreign key constraint of table flarum/#sql-11ba_5c:
foreign key (`user_id`) references `users` (`id`) on delete cascade:
Cannot resolve table name close to:
(`id`) on delete cascade

What could that be?

The table name close to: (`id`) on delete cascade, shouldn't that be the users table? And if so, why couldn't the table name be resolved?

I will investigate further...

Ralkage

Pollux You could of also dropped the column VIA ALTER TABLE statement.

ALTER TABLE "table_name"
DROP "column_name";

Given that the column is not part of core (which it looks to be).

@luceos any particular reason why ->onDelete('cascade');is used in up portion of the migration and not down?

luceos

Ralkage onDelete('cascade') is a Laravel method that simply tells MySQL to delete related records whenever the foreign record is deleted. The down method should have deleted the foreign key constraint, which is possible using dropForeign.

Pollux

Ralkage Given that the column is not part of core (which it looks to be).

That's the part of the migration we are talking about:

2018_04_30_create_flagrow_terms_policy_user.php

$table->foreign('policy_id')->references('id')->on('flagrow_terms_policies')->onDelete('cascade');
$table->foreign('user_id')->references('id')->on('users')->onDelete('cascade');

So the first foreign key (policy_id) refers to the id column in the flagrow_terms_policies table (parent). That's not so important as the flagrow_terms_policiestable gets deleted as well. The second foreign key (user_id) however refers to the id column in the userstable. This foreign key needs to be removed, the column can't simply be dropped.

Do I understand that correctly?

Ralkage any particular reason why ->onDelete('cascade'); is used in up portion of the migration and not down?

Delete actions are tied to a foreign key constraint, these are defined on up. What do you want to do on down? On down I would rather expect the removal of the foreign key constraints added by the migration.

Edit: I just saw, that @luceos already explained my last point.

clarkwinkelmann

Pollux never thought this could get so complex ?

Did you mention which MySQL version you're using ? Or is it MariaDB ?

From your error log it looks like the foreign key creation command was interpreted as a syntax error. Which is very strange. That's not the only Flagrow extension that's using foreign keys either.

The SQL syntax to add a foreign key looks something like this:

ALTER TABLE my_table ADD CONSTRAINT my_key FOREIGN KEY ('some_id')  REFERENCES some_new_table ('some_other_id') ON UPDATE CASCADE ON DELETE CASCADE;

So it looks like the error reported is't happy with the reference table name which should be just before the ('id') part. Do you have a way to see the full SQL that it tried to execute ?

You can also see from the command syntax that a foreign key is a table constraint, so it goes away if you delete the table.

Pollux

Finally I could install the extension.

I was stuck in a state, were parts of the installation had failed. The flagrow_terms_policies table had been missing and I had to a) manually remove the 2018_04_30_create_flagrow_terms_policies entry in the migrations table and b) flagrow-terms from the extensions_enabled entry in the settings table and c) re-activate the extension in the Admin panel.

Why I ran into these problems, I still have no idea.

clarkwinkelmann Did you mention which MySQL version you're using ? Or is it MariaDB ?

Database server

Server: Localhost via UNIX socket
Server type: MySQL
Server version: 5.7.18 - MySQL Community Server (GPL)
Protocol version: 10
Server charset: UTF-8 Unicode (utf8)

Web server

Apache/2.4.28 (Unix) PHP/5.6.30
Database client version: libmysql - mysqlnd 5.0.11-dev - 20120503 - $Id$
PHP extension: mysqliDocumentation curlDocumentation mbstringDocumentation
PHP version: 5.6.30

clarkwinkelmann That's not the only Flagrow extension that's using foreign keys either.

Which extensions do you have in mind? Maybe I don't have any of those installed.

clarkwinkelmann Do you have a way to see the full SQL that it tried to execute ?

Unfortunately I couldn't find that out.

Two things that drew my attention during a first test of the Terms extension:

a) There should be a way for normal users to cancel the Terms Update modal and be logged out again. I think of a situation, where users don't have enough time to study the new terms at a given time. In this case they should be able to cancel their login and not having to resort to deleting the Flarum cookies.

b) I'm probably not the only one who uses Sijads flarum-ext-pages extension for the terms. Right now my new terms can only be inspected after they are accepted. As these pages are also visible to not-logged-in visitors they should probably be excluded from the invocation of the Terms Update modal.

clarkwinkelmann

Pollux very good points indeed.

Right now you can allow anybody to skip the acceptation, this will give them access to the logout and pages.

Maybe we should allow closing the modal even when we blocked access to all features, that way it's still possible to do those. My concern was that users would probably see a modal, close it without reading and then complain they can't do anything on the forum...

A somehow pretty way to fix it: I could add a hero-style banner at the top of the website saying you have to accept the new terms before you can do anything, just like the confirm email alert. Clicking it would open the existing modal. And the modal could automatically be opened on the homepage only.

Would that solve those issues ?

RelatedTitle

FriendsOfFlarum Thank you Ive been looking for something like this ?

Pollux

clarkwinkelmann Right now you can allow anybody to skip the acceptation, this will give them access to the logout and pages

That's what I will do first for a couple of days, but at some point I need to make accepting the new rules mandatory.

Maybe we should allow closing the modal even when we blocked access to all features, that way it's still possible to do those. My concern was that users would probably see a modal, close it without reading and then complain they can't do anything on the forum...

That seems reasonable to me. Users are technically still logged in, but they can act only like guests. As you said, it will be necessary to make it very clear why the access to user functions is limited.

A somehow pretty way to fix it: I could add a hero-style banner at the top of the website saying you have to accept the new terms before you can do anything, just like the confirm email alert. Clicking it would open the existing modal. And the modal could automatically be opened on the homepage only.

Would that solve those issues ?

That sounds good to me.

I still don't understand why update dates have to be in the past. While the date is in the future the alert could be unobtrusive and allow postponement for everyone, once the given date has been reached, the behaviour changes to the one we see now.

Why would this be a problem? The help text says:

Setting this value will require all users who accepted prior to that date to accept again.
Must be in the past (otherwise users would never be able to accept after the date and get past the modal).

Every unaccepted policy update triggers a modal. Once all updates are accepted, no modal will be shown. If the same policy gets another update, wouldn' this be unrelated to previous updates? If so, what is the critical importance of the date then?

Or do you expect that new updates of the same policy will overwrite older updates? This sounds like a good idea; in this case a versioning of the updates would solve the problem.

clarkwinkelmann

I added a Compatibility section to the first post.

So far the following can be confirmed:

All Flagrow extensions should be compatible (Terms is the only one extending the sign up modal)
Works with Sijad's reCAPTCHA
Does not work with ReFlar User Management. Even if you didn't enable the age or gender fields. See ReFlar/user-management20 for more

luceos

clarkwinkelmann Does not work with ReFlar User Management. Even if you didn't enable the age or gender fields. See ReFlar/user-management#20 for more

you can use the conflict key in composer: https://getcomposer.org/doc/04-schema.md#conflict

clarkwinkelmann

Pollux there's always room for improvements ?

The "terms updated at" value should be seen as "everybody who accepted prior to that date now has to accept again". Meaning, it has to be in the past or everybody will always be matched and nobody will be able to pass ?

I wanted to implement some kind of versioning, but it's a bit more complex. I'd need to save the individual revisions, which revision was accepted by a user and at what time a revisions must be accepted. The problem being that if say you release a new version of the revision each month and have one month to accept it, if I do not save which version a user agreed to, the user would always be in the grace period and doesn't need to accept anything.

If somebody's willing to sponsor the revision feature I will gladly implement it ?

Regarding the ability to close the modal while still being blocked I will try to implement it as soon as possible.

Pollux

clarkwinkelmann The "terms updated at" value should be seen as "everybody who accepted prior to that date now has to accept again". Meaning, it has to be in the past or everybody will always be matched and nobody will be able to pass ?

First I understood your extension in a way that each update would have its own entry in the flagrow_terms_policies table. I later understood that this is not the way your extension works and also that this approach wouldn't make a lot of sense either. The way you've organized the policies makes a lot more sense, but now you have to know which version of a given policy has been accepted and this can be achieved by comparing the date of the last version and the date of the last acceptance. A lean and simple approach with the only drawback of missing grace periods. But that's absolutely OK with me.

I wanted to implement some kind of versioning, but it's a bit more complex.

Thinking about versions of policies and grace periods, I realised too that this get's complicated soon. Let's for example look at the situation, where a user has accepted version 5, version 6 is due and version 7 is announced (grace period). Should we offer both versions 6 and 7 for acceptance? Should we offer version 7 first and in case of decline come up with version 6 which can't be declined? What about overlapping grace periods? This can easily become very confusing.

clarkwinkelmann Regarding the ability to close the modal while still being blocked I will try to implement it as soon as possible.

Great.

I have found one bug in my installation, but I'm not sure whether this is due to my difficulties during installation. If I check the Terms accept state of a user, I can't see the accept date, it just shows:

Policy 1: Accepted at ,[object Object]
Policy 2: Not accepted

Policy 1 had been accepted before, policy 2 had not.

Edit: The date in the database looks unsuspicious to me:

 policy_id    user_id    accepted_at
	 1	    1 	 2018-05-11 09:37:06

clarkwinkelmann

luceos you can use the conflict key in composer:

@Ralkage is already adding the conflict key to User Management. We could add it as well.

Pollux Should we offer both versions 6 and 7 for acceptance? Should we offer version 7 first and in case of decline come up with version 6 which can't be declined? What about overlapping grace periods? This can easily become very confusing.

Indeed. The current version is the only simple solution that works no matter what I guess.

Pollux Accepted at ,[object Object]

I will have to investigate that... I thought that was handled.

« Previous Page Next Page »