So I had a couple of hours and I needed this module's functionality. So first of all, thank you for the module :)
I have decided to rewrite for the following reasons:

1) I think it's better to have a single table for the module.
2) There was a lot of code duplication, that I've moved into API functions.
3) Code is more readable (or at least I hope).

I haven't written an upgrade path from the previous tables to the new one, and I'm I won't have the time to do it, nor much interest.

CommentFileSizeAuthor
#18 Edit view user_role_expire | mac pirc_1248677241378.png693.8 KBstewsnooze
#6 role-expire-6.patch27.69 KBamitaibu
#2 role_expire-rewrite-2.patch25.87 KBamitaibu
#1 role-expire-rewrite-1.patch25.83 KBamitaibu
role-expire-rewrite.patch25.82 KBamitaibu

Comments

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
StatusFileSize
new role-expire-rewrite-1.patch25.83 KB

small fix in a form merge.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
StatusFileSize
new role_expire-rewrite-2.patch25.87 KB

Fix it for real this time...

bdziewierz’s picture

bdziewierz commented

Thank you for this submission! I am looking at the code now.

bdziewierz’s picture

bdziewierz commented
Status: Needs review » Needs work

Amitai, thank you for you work on this. The cleanup and Actions hook are all definately helpful, but... Let me explain the reason behind using multiple database tables:

Obviously when I started to code this module I used just one table (exactly the same structure you're proposing), but quickly it turned out that in order to integrate the the module with Views (as it is at the moment with v1.1), complicated handler will be required. It also turned out, that this can be easily done with default handlers, but separate table for each role will be required. I picked the second option and went multiple tables route.

Your patch joins multiple tables back to one, but it looks like it ignores Views integration. After applying it, Views part of the module becomes disfunctional.

I will keep reviewing the code and commit the parts of the patch that won't break Views integration... or try to overcome Views problem somehow. If you have any suggestions, let me know! Thanks!

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

>>Your patch joins multiple tables back to one, but it looks like it ignores Views integration

1) Indeed, I haven't looked at the Views integration yet. Anyway, it sounds like the wrong reason to not unify the tables into one - I don't know any other module (except of CCK, but it's a different story), that needs multiple tables.
2) Views is an important feature, that can be handled in a follow up patch. But IMO, if the main functionality is wrong, we should deal with it first.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented
Status: Needs work » Needs review
StatusFileSize
new role-expire-6.patch27.69 KB

Ok, Here's a patch with the Views fix. You can check it by creating a View with the base table = Users. It's not throughly tested, but already shows we don't need multiple tables :)

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

@bdziewierz,
Did you have a chance to review this issue? I'm trying to push the user of the module here.

bdziewierz’s picture

bdziewierz commented

Hi Amitaibu,

The whole process should be completed by Weds.

Regards,
B.

bdziewierz’s picture

bdziewierz commented
Status: Needs review » Needs work

The Views part of the module still doesn't work as intended, unfortunatelly. As you can see - after installing the patch - under Views' filters, fields or sort there's only one role expire option. The question is, which role it refers to? For example, how can I show expiration date of role x of user y? Or how can I filter based on expiration date of role x of user y? At the moment, patched module doesn't allow you to do so.

I am writing custom Views handlers to support this (and to support this #423744: Filtering for absent expiration also). Once this will completed, version 1.2 that will include your rewrite will be launched.

bdziewierz’s picture

bdziewierz commented
Assigned: amitaibu » bdziewierz
amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

Cool, 10x for the update.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

I had a quick look in the CVS, two things:
1) I'd appreciate credit on the rewrite, like it's done on other Drupal projects :).
2) You have the _role_expire_get_role() helper function you can use (for example in the module's install file).

stewsnooze’s picture

stewsnooze
he / him
commented

Any news on this? It's in CVS, any release planned? any remaining bugs you need help with?

bdziewierz’s picture

bdziewierz commented
Assigned: bdziewierz » Unassigned
Priority: Normal » Critical

I am running short of free time to work on this module recently. Too much work! If anyone interested in co-maintaing the module with me then I will accept any help! Just let me know or submit a patch and, once accepted, I will give you CVS access. @Amitaibu, are you interested?

bdziewierz’s picture

bdziewierz commented

At the moment we need Views support for this patch: role-expire-6.patch.

stewsnooze’s picture

stewsnooze
he / him
commented

I'm interested in taking on this module as I want to use it on Economist.com. I'll try to patch the views support in. Happy to share it with others of course.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

@bdziewierz,
I haven't got much time myself... You can grant me CVS rights, and when I'll have time I'll go over it.

stewsnooze’s picture

stewsnooze
he / him
commented
StatusFileSize
new Edit view user_role_expire | mac pirc_1248677241378.png693.8 KB

The views integration works.

Could it be that when you updated cvs you didn't go to update.php?

I tried with a fresh install and updating and it worked fine.

I've attached a screenshot of a working view (taken from HEAD)

stewsnooze’s picture

stewsnooze
he / him
commented

hi,

any news on committing this great patch. If you need help I am happy to also become a maintainer. If you don't have much time I am sure either Amitaibu or I would take it. I'm keen to use this but only in it's patched form.

stewsnooze’s picture

stewsnooze
he / him
commented
Status: Needs work » Reviewed & tested by the community
bdziewierz’s picture

bdziewierz commented

@stewsnooze: I guess you need CVS access to become co-maintainer.
@amitaibu: I've granted you CVS access for this project so if you have some spare time we will all be grateful for your help! Thanks!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

@bdziewierz: Do you accept stewsnooze's offer to become co-maintainer? If you accept his offer, then he can get a CVS account.

bdziewierz’s picture

bdziewierz commented

@KiamLaLuno: Yes, please.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I approved stewsnooze's CVS application, and granted him access to this project repository.

stewsnooze’s picture

stewsnooze
he / him
commented

Hi,

My CVS account is active now. Can you please set me as a maintainer and I will happily make a release including this issue.

Thanks

stewsnooze’s picture

stewsnooze
he / him
commented

Actually I can see that I have access already now. At least I have an edit button!

stewsnooze’s picture

stewsnooze
he / him
commented
Version: 6.x-1.1 » 6.x-1.2
Status: Reviewed & tested by the community » Fixed

Committed and released in http://drupal.org/node/546932

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.