Skip to content

Bug2152 prime #312

Closed
wants to merge 5 commits into from

4 participants

@louiseadennis

A rebased version of Bug2152. Ready for (as yet unmade) changes requested from code review.

@zorkian
Dreamwidth Studios member
zorkian commented Mar 11, 2013

Hiya; this still has a commit on it we aren't looking for.

I think this is because your develop branch is not up to date, so when you rebased on top of Dreamwidth's develop, it included the commit from 90d.

I would suggest:

$ git checkout develop
$ git pull --ff-only dreamwidth develop:develop
$ git push origin develop
$ git checkout Bug2152_prime
$ git rebase develop
$ git push origin Bug2152_prime

Then come back to this pull request and see if anything has changed? And of course, if anything errors out in those steps, please stop and let me/Afuna know what it says?

@louiseadennis

My local develop branch was up-to-date, but my branch on github was behind. I ran through the above sequence and all was fine (give or take a few Already-up-to-date message) until the final step where it failed with

dh-purplecat@newhack:~/dw$ git push origin Bug2152_prime
Password for 'https://annariel@github.com':
To https://annariel@github.com/annariel/dw-free.git
! [rejected] Bug2152_prime -> Bug2152_prime (non-fast-forward)
error: failed to push some refs to 'https://annariel@github.com/annariel/dw-free.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@afuna
Dreamwidth Studios member
afuna commented Mar 13, 2013

So in this case, your develop branch on github has an extra commit https://github.com/annariel/dw-free/commits/Bug2152_prime -- the second one there says that it was commited by purplecat

Normally if there are extra commits on the branch that you're pushing to, you want to first pull them in. But in this case, you want to get rid of them -- so do:

git push -f 

and that will force the github branch to be like your local branch.

@louiseadennis

Thank goodness, that finally seems to have worked. I'll start working on the code review now!

@afuna afuna commented on the diff Mar 22, 2013
cgi-bin/DW/Setting/StickyEntry.pm
+ foreach my $i ( 1... $u->count_max_stickies ) {
+ my $url = "http://$username.dreamwidth.org/$stickies[$i - 1].html" if $stickies[$i - 1];
+ my $textentry = $errs ? $class->get_arg( $args, "stickyid${i}" ) : $url;
+
+ $ret .= "<label for='${key}stickyid${i}'>" . $class->ml( 'setting.stickyentryi.label' ) . " $i </label>";
+ $ret .= LJ::html_text({
+ name => "${key}stickyid${i}",
+ id => "${key}stickyid${i}",
+ class => "text",
+ value => "$textentry",
+ size => 50,
+ maxlength => 100,
@afuna
Dreamwidth Studios member
afuna added a note Mar 22, 2013

I think that works a lot better now! In addition, I'd like to suggest that, if there's no value, a "placeholder" attribute be put into place, as a hint to anyone putting in their first sticky entries (though hopefuly they'll just do it via the entry page!)

@louiseadennis
louiseadennis added a note Apr 2, 2013

I've added (Entry Number) in place of the ditemid in the first "blank" entry. Commit: louiseadennis@2f99b05

@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Ah I just realized that I mentioned this before, but was likely unclear :)

Try putting a:

placeholder => "http://.../1234.html"

in the same place the name/id/value, etc got set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
purplecat added some commits Jan 23, 2013
purplecat Bug 2152 - Multiple stickies bugs.dwscoalition.org/show_bug.cgi?id=2152
Allowing multiple stickies to be set both in the post forms, via a sticky module and in the Display tab under Manage account.
Allows for different sticky allocations depending upon account type and attempts to handle switching between account types elegantly.
145cc3e
purplecat (Bug 2152) Made changes to User.pm following on from code review disc…
…ussion.

(github.com/dreamwidth/dw-free/pull/276/files#r3386924)
6c95934
purplecat (Bug 2152) Made changes to Entry.pm following on from code review dis…
…cussion.

This involved alterations to module-sticky.tt, most notably to change the value of "sticky_select" form elements
so that their results could be cast into an integer describing the desired position of the sticky (O indicating the
entry is not a sticky).  This made the use of the form element more intuitive in Entry.pm.  Previously sticky
position had to be determined by comparing the ditemid of an edited entry with that of sticky entries, not it
can be determined from the form variable.  Variables in Entry.pm were re-named accordingly where appropriate.
8145b48
purplecat (Bug 2152) Making changes to StickyEntry.pm requested during code rev…
…iew.

This includes a change to the way is_sticky in User.pm handles its inputs.

This commit also includes a bug fix to Entry.pm
d4bf53a
purplecat (Bug 2152) Final changes requested by code review.
This has involved considerable extension and reworking of the entry from interface
for the stickies to handle the various combinations of journals and posters that
are possible from that form.
2f99b05
@louiseadennis louiseadennis referenced this pull request Apr 2, 2013
Closed

Bug2152 #276

@louiseadennis

This should be ready for a new code review now.

@zorkian zorkian added this to the Pull Requests milestone Mar 14, 2014
@afuna afuna commented on the diff Apr 7, 2014
views/entry/module-sticky.tt
+ id = "issticky"
+ checked = 1
+ value=1
+ );
+ ELSE;
+ form.checkbox( label = label_yes
+ name = "issticky"
+ id = "issticky"
+
+ value=1
+ );
+ END;
+ %]
+ </div>
+ <div id='sticky_positions'>
+ [%# first_unused_sticky = 0 if there are no free sticky options
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmmm, what if instead of a list of radioboxes which looks ilke so:

Sticky Position: () Sticky 1
                    Currently: 1618 (No Subject)
                 () Sticky 2

Use textboxes and emphasize the subject instead:

[ ] Make Sticky Entry
(expanded section)
[ 1 ] this entry
[ 2 ] (No Subject) - 1618
[ 3 ] Some entry with a subject

With the assumption / default being that we want to make this entry be the topmost sticky entry, and an option to reorder.

Actually, for simplicity's sake, how do you feel about just having:

[ ] Sticky to top of journal/community

And if that's checked, then expand to show a link to the accounts page to reorder, with some indication that this will become the topmost. That might balance the desire to keep the flow simple with the ability to have some flexibility.

With an assumption made that we want this entry to be the top most sticky entry

@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Status message: Sticky #1 - reorder

Where reorder is a link?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Controller/Entry.pm
@@ -231,6 +237,7 @@ sub new_handler {
trust_datetime_value => $trust_datetime_value,
crosspost => \%crosspost,
+ sticky_pos => int( $sticky_pos ),
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Small detail: I'd prefer to have the conversion to int happen where we set the variable, because then that way we know that if we need to use $sticky_pos anywhere else in the future, that'll be consistent/safe.

I like that presence of the checking though -- just in case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Controller/Entry.pm
@@ -328,10 +338,15 @@ sub _init {
}
}
-
- @journallist = ( $u, $u->posting_access_list )
- unless $usejournal;
-
+ unless ( $usejournal ) {
+ my @postables = $u->posting_access_list;
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Ahhh I see what you're doing here! For clarity, I'd suggest moving the my @postables line to above the for loop (or even just use that in the for loop directly); but logic looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Controller/Entry.pm
+ my $max_stickies = $journalu->count_max_stickies;
+ my $entry_is_sticky = 0;
+ for my $i ( 0... $max_stickies-1 ) {
+ my $sticky = $journalu->get_sticky_entry( $i );
+ if ( $sticky ) {
+ my $subject = $sticky->subject_html;
+ my $sticky_name = $subject ? $subject : "No Subject";
+ $checked = 1 if ( $i + 1 == $sticky_pos );
+ $entry_is_sticky = $checked if $checked;
+ push @stickylist, { name => $sticky_name, ditemid => $sticky->ditemid, position => $i + 1, issticky => 1, checked => $checked };
+ $checked = 0;
+ # If all allowed sticky positions are taken then the first unused sticky is 0
+ # i.e., there isn't one.
+ $first_unused_sticky = $i == $max_stickies - 1 ? 0 : $i + 2;
+ } else {
+ push @stickylist, { name => "", ditemid => 0, position => $i + 1, issticky => 0, checked => 0 }
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

ponder I think that an empty sticky name ends up being confusing unless you understand that the sticky is meant to refer to this entry -- so maybe we can say current entry, or similar -- but see my comment on the frontend template below, this may be moot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Controller/Entry.pm
@@ -412,6 +449,10 @@ sub _init {
moodtheme => \%moodtheme,
moods => \@moodlist,
+ stickies => \@stickylist,
+ first_unused_sticky => $first_unused_sticky,
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hrrrm. I think that $first_unused_sticky, it makes most sense to have us assume that they want this latest sticky to go on top, instead of in the first blank.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Controller/Entry.pm
@@ -1069,8 +1131,19 @@ sub _do_edit {
my $juser = $journal->user;
my $entry_url = $res->{url};
my $edit_url = "$LJ::SITEROOT/entry/$juser/$ditemid/edit";
+ my $u = $auth->{poster};
+ my $ju = $auth->{journal} || $auth->{poster};
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Created $u and $ju but never used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Controller/Entry.pm
@@ -1069,8 +1131,19 @@ sub _do_edit {
my $juser = $journal->user;
my $entry_url = $res->{url};
my $edit_url = "$LJ::SITEROOT/entry/$juser/$ditemid/edit";
+ my $u = $auth->{poster};
+ my $ju = $auth->{journal} || $auth->{poster};
+ my $sticky_pos = $form_req->{props}->{sticky_select};
+
+ if ( $sticky_pos == 0 && $journal->is_sticky_entry( $ditemid ) ) {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Ponder ponder. Okay this if statement is slightly convoluted. How about:

if ( $sticky_pos ) {
    $journal->make_sticky_entry(....)
} else {
    $journal->remove_sticky_entry($ditemid) if $journal->is_sticky_entry(...)
}

Which basically just reverses it and gets rid of one negative condition which is always a bit harder to parse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
+ foreach my $i ( 1... $u->count_max_stickies ) {
+ my $url = "";
+ if ( $stickies[$i - 1] ) {
+ $url = "http://$username.dreamwidth.org/$stickies[$i - 1].html";
+ } else {
+ $url = "http://$username.dreamwidth.org/(Entry Number).html" if ( $i == 1 || $stickies [$i - 2] );
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Ooooh I like this... but I suggest that in this case, you use this as a placeholder HTML attribute, so that when someone clicks on the textbox, the example disappears without them having to select and delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
return $ret;
}
sub save {
my ( $class, $u, $args ) = @_;
- my $sticky = $class->get_arg( $args, "stickyid" ) || '';
- $sticky = LJ::text_trim( $sticky, 0, 100 );
- unless ( $u->sticky_entry ( $sticky ) ) {
- $class->errors( "stickyid" => $class->ml( 'setting.stickyentry.error.invalid' ) ) ;
+ my $max_sticky_count = $u->count_max_stickies;
+ my @stickies;
+ # Create a hash that we will use to check for duplicate entries.
+ my %unique = ();
+ my $username = $u->user;
+ my $defaulturl = "http://$username.dreamwidth.org/(Entry Number).html";
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

We could probably set this variable earlier and use it in both instances Just in Case (future consistency)

(though actually looking below, we may not need it, because we're only using the defaulturl to ignore the form submission if they didn't change anything -- but having the sample as a placeholder instead of the actual form text, means we won't need to do this!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
return $ret;
}
sub save {
my ( $class, $u, $args ) = @_;
- my $sticky = $class->get_arg( $args, "stickyid" ) || '';
- $sticky = LJ::text_trim( $sticky, 0, 100 );
- unless ( $u->sticky_entry ( $sticky ) ) {
- $class->errors( "stickyid" => $class->ml( 'setting.stickyentry.error.invalid' ) ) ;
+ my $max_sticky_count = $u->count_max_stickies;
+ my @stickies;
+ # Create a hash that we will use to check for duplicate entries.
+ my %unique = ();
+ my $username = $u->user;
+ my $defaulturl = "http://$username.dreamwidth.org/(Entry Number).html";
+ for ( my $i=1; $i<=$max_sticky_count; $i++ ) {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

FYI, we can also write this as:

for my $i ( 1..$max_sticky_count ) ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
+ my $max_sticky_count = $u->count_max_stickies;
+ my @stickies;
+ # Create a hash that we will use to check for duplicate entries.
+ my %unique = ();
+ my $username = $u->user;
+ my $defaulturl = "http://$username.dreamwidth.org/(Entry Number).html";
+ for ( my $i=1; $i<=$max_sticky_count; $i++ ) {
+ my $stickyi = $class->get_arg( $args, "stickyid${i}" ) || '';
+ # Unless this is a blank form entry...
+ unless ( $stickyi eq '' || $stickyi eq $defaulturl ) {
+ $stickyi = LJ::text_trim( $stickyi, 0, 100 );
+ my $ditemid = $u->is_valid_entry( $stickyi );
+ # is_valid_entry will return the correct itemid if a URL has been given. It will be
+ # undefined if the text box entry has a problem.
+ if ( $ditemid ) {
+ if ( exists $unique{ $ditemid } ) {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

style nitpick: don't need the spaces in $unique{$ditemid}

(Also, we don't need the "exists", since we can go with the truth/false value of $unique{$ditemid})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
+ my @stickies;
+ # Create a hash that we will use to check for duplicate entries.
+ my %unique = ();
+ my $username = $u->user;
+ my $defaulturl = "http://$username.dreamwidth.org/(Entry Number).html";
+ for ( my $i=1; $i<=$max_sticky_count; $i++ ) {
+ my $stickyi = $class->get_arg( $args, "stickyid${i}" ) || '';
+ # Unless this is a blank form entry...
+ unless ( $stickyi eq '' || $stickyi eq $defaulturl ) {
+ $stickyi = LJ::text_trim( $stickyi, 0, 100 );
+ my $ditemid = $u->is_valid_entry( $stickyi );
+ # is_valid_entry will return the correct itemid if a URL has been given. It will be
+ # undefined if the text box entry has a problem.
+ if ( $ditemid ) {
+ if ( exists $unique{ $ditemid } ) {
+ $class->errors( "stickyid" => ( $class->ml( 'setting.stickyentry.error.duplicate' ) . $i ) ) ;
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmmm. For translation strings with variables, it's better to pass the variable to the translation string, instead of appending to the end -- this helps in case we need to change the text of the string, and the variable is no longer right at the end. so the way to do this is:

$class->ml( 'setting.stickyentry.error.duplicate', { stickyid => $i } )

And then have your text string be:

setting.stickyentry.error.duplicate=etc etc [[stickyid]] etc etc etc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
+ # Unless this is a blank form entry...
+ unless ( $stickyi eq '' || $stickyi eq $defaulturl ) {
+ $stickyi = LJ::text_trim( $stickyi, 0, 100 );
+ my $ditemid = $u->is_valid_entry( $stickyi );
+ # is_valid_entry will return the correct itemid if a URL has been given. It will be
+ # undefined if the text box entry has a problem.
+ if ( $ditemid ) {
+ if ( exists $unique{ $ditemid } ) {
+ $class->errors( "stickyid" => ( $class->ml( 'setting.stickyentry.error.duplicate' ) . $i ) ) ;
+ return 1;
+ }
+ push( @stickies, $ditemid );
+ $unique{ $ditemid } = 1;
+ } else {
+ # As soon as we detect a problem with a sticky we break out of the subroutine.
+ $class->errors( "stickyid" => ( $class->ml( 'setting.stickyentry.error.invalid2' ) . $i ) ) ;
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Yep. (to the comment. But also same translation string advice as above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/DW/Setting/StickyEntry.pm
}
+
+ # We pass in a reference to the array - which will be a reference to an empty array if the user has
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hrrrm I think this comment is unneeded, because we have a bunch of other methods which are getters/setters at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/S2/RecentPage.pm
my $stickyentry;
- $stickyentry = $u->get_sticky_entry
- if $skip == 0 && ! $opts->{securityfilter} && ! $opts->{tagids};
- # only show if visible to user
- if ( $stickyentry && $stickyentry->visible_to( $remote, $get->{viewall} ) ) {
- # create S2 entry object and show first on page
- my $entry = Entry_from_entryobj( $u, $stickyentry, $opts );
- # sticky entry specific things
- my $sticky_icon = Image_std( 'sticky-entry' );
- $entry->{_type} = 'StickyEntry';
- $entry->{sticky_entry_icon} = $sticky_icon;
- # show on top of page
- push @{$p->{entries}}, $entry;
+ if ( $skip == 0 && ! $opts->{securityfilter} && ! $opts->{tagids} ) {
+ for ( my $i = 0; $i < $u->count_max_stickies; $i++ ) {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Logic looks good!

(see previous comment on using a range in a for loop too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
+sub is_sticky_entry {
+ my ( $u, $ditemid ) = @_;
+
+ my @stickies = $u->sticky_entry;
+
+ for ( my $i = 1; $i<=$u->count_max_stickies; $i++ ) {
+ return $i if ( $ditemid == $stickies[$i-1] );
+ }
+ return 0;
+}
+
+# Checks if some input is a valid URL or ID for an entry in the users journal, returns
+# the entry itemid if it is valid.
+#
+# Created to support multiple stickies but can probably replace duplicate code
+# in several over places.
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

I like this reduction of code duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
@@ -2641,6 +2682,22 @@ sub large_journal_icon {
return $wrap_img->( "user" );
}
+# Make a particular entry into a particular sticky.
+sub make_sticky_entry {
+ my ( $u, $ditemid, $sticky_id ) = @_;
+
+ return undef if $sticky_id > $u->count_max_stickies;
+
+ my $sticky_entries = $u->prop( 'sticky_entry' );
+ my @stickies = split( /,/, $sticky_entries );
+
+ $stickies[$sticky_id-1] = int( $ditemid );
+ my $sticky_entry = join( ',', @stickies );
+
+ $u->set_prop( sticky_entry => $sticky_entry );
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmmmm. Why not use $u->sticky_entry below?

@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Oh! Does this replace existing stickies in an index completely instead of moving them aside? I think that makes sense in the account settings, but not so much for the update page.

How about having both a make_sticky_entry and prepend/push_sticky_entry for both use cases? A little bit extra work but..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
@@ -2952,6 +3009,23 @@ sub remove_from_class {
return $u->modify_caps( [], [$bit] );
}
+# Remove a particular entry from the sticky list
+sub remove_sticky_entry {
+ my ( $u, $ditemid ) = @_;
+
+ my $sticky_entries = $u->prop( 'sticky_entry' );
+ my @stickies = split( /,/, $sticky_entries );
+
+ my @new_stickies;
+
+ @new_stickies = grep { !/$ditemid/ } @stickies;
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmmm. Almost, but we'll want the comparison to be more exact (what if they have both a "234" and "123456" in their list of stickies?

We can use a direct comparison here: grep { $_ != $ditemid }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
@@ -3172,40 +3246,83 @@ sub thread_expand_all {
}
#get/set Sticky Entry parent ID for settings menu
+# Expects an array of entries as input
+# Returns an array of entries
+# NB. This assumes that the inputs have already been validated as actual journal entries otherwise bad things
+# will happen.
sub sticky_entry {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Maybe we should call this sticky_entries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
sub sticky_entry {
- my ( $u, $input ) = @_;
+ my ( $u, $input_ref ) = @_;
+
+
+ # The user may have previously had an account type that allowed more stickes.
+ # we want to preserve a record of these additional stickes in case they once
+ # more upgrade their account. This means we must first extract these
+ # if they exist.
+ my $sticky_entries = $u->prop( 'sticky_entry' );
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

I like the thought put into this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
- if ( defined $input ) {
- unless ( $input ) {
+ unless ( $input[0] ) {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmm maybe check the length instead of item0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
- # Validate the entry
- my $item = LJ::Entry->new( $u, ditemid => $ditemid );
- return 0 unless $item && $item->valid;
+ # sanity check the elements of the input array of candidate stickies.
+ my $new_sticky_count = 0;
+ foreach my $stickyid ( @input ) {
+ $new_sticky_count++;
+ return undef unless ( defined $u->is_valid_entry( $stickyid ) );
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

I think defined is not necessary here (unless there's a specific reason for it to be!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
@@ -3172,40 +3246,83 @@ sub thread_expand_all {
}
#get/set Sticky Entry parent ID for settings menu
+# Expects an array of entries as input
+# Returns an array of entries
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hrrrrm. The comment says that it returns an array of entries, but sometimes we return 1 (if it was used as a setter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
sub get_sticky_entry {
- my $u = shift;
+ my ( $u, $i ) = @_;
+
+ return undef unless ( defined $i );
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

style nitpick: don't need the parentheses here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
}
+# Get's ith sticky entry from list
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

psst don't need apostrophe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
+ my ( $u, $ditemid ) = @_;
+
+ my @stickies = $u->sticky_entry;
+
+ for ( my $i = 1; $i<=$u->count_max_stickies; $i++ ) {
+ return $i if ( $ditemid == $stickies[$i-1] );
+ }
+ return 0;
+}
+
+# Checks if some input is a valid URL or ID for an entry in the users journal, returns
+# the entry itemid if it is valid.
+#
+# Created to support multiple stickies but can probably replace duplicate code
+# in several over places.
+sub is_valid_entry {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Oh! I just remembered, there is a LJ::Entry->new_from_url(...) method, which seems relevant here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
cgi-bin/LJ/User.pm
@@ -2614,6 +2618,43 @@ sub invalidate_directory_record {
undef, $u->id);
}
+# checks whether an entry id corresponds to that of a sticky entry which is under user's max_sticky_count. Returns the position of the sticky if it is one.
+sub is_sticky_entry {
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmmm since this returns a pos instead of a true/false, I suggest renaming it to sticky_entry_index, or something similar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
htdocs/js/jquery.postform.js
+ if ( !$( "#issticky" ).is(":checked") )
+ $( "#sticky_positions" ).hide();
+
+ $( "#post_entry" ).bind( "journalselect", function(e, journal ) {
+ var warning_class = "sticky_msg_warning";
+ var $warning = $( "#"+warning_class );
+ var $posting_as_other = $( "#post_as_other").is(":checked");
+
+ if ( journal.name && journal.isremote && !$posting_as_other) {
+ $( "#sticky_component" ).sticky( "toggle", "community", ! journal.iscomm || journal.has_admin);
+ if ( $( "#usejournal").is("select") && journal.iscomm && journal.has_admin ) {
+ // In this situation although the user has sticky privileges for the journal they have
+ // not been pre-loaded onto the page.
+ if ( $warning.length == 0) {
+ $( "#sticky_component" ).sticky( "toggle_sticky_checked_options", false );
+ var $p = $( "<p></p>", { "class": warning_class, "id": warning_class } ).text( "WARNING: Unable to determine sticky settings for this community from this page. Checking Make Sticky Entry will make this entry into the first sticky on the journal and replace that sticky if it is already set. If that is not the desired behaviour please edit from either Account Settings->Display or by posting to the community directly" );
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hrrrm. I like the thought put into this edge case, but I think it would be easier if we just don't give them the option to do this, if we can't detect if they're admin or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna afuna commented on the diff Apr 7, 2014
views/entry/module-sticky.tt.text
@@ -0,0 +1,9 @@
+;; -*- coding: utf-8 -*-
+.header=Sticky Entries
+
+.label=Sticky [[position]]
+.label.sticky_pos=Sticky Position:
+.label_yes=Make Sticky Entry
+.sticky_label=Currently: [[num]] ([[name]])
+.sticky_position_req=All possible stickies are used. Please select one to replace.
+.is_sticky_warning=Please note altering the position of an existing sticky may cause the positions of other stickies to change.
@afuna
Dreamwidth Studios member
afuna added a note Apr 7, 2014

Hmm but currently don't we just overwrite the other sticky at that position instead of changing the positions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@afuna
Dreamwidth Studios member
afuna commented Apr 7, 2014

I'm sorry it took so long!

I think the logic is mostly good; I've suggested a couple refinements but I didn't have much to say about the saving/fetching part, etc.

Most of my comments were interface/workflow related -- my biggest concern is overwriting the sticky at the current index, for the update page (vs the account settings). I think that many of the things I said are related to this behavior, so figuring this out is the most important part, because once this is fixed most of the things I brought up become moot.

@zorkian
Dreamwidth Studios member
zorkian commented Jan 29, 2015

Hiya! I'm going to place this up for grabs, but if you have any interest in getting back to it, please let us know!

@louiseadennis louiseadennis was unassigned by zorkian Jan 29, 2015
@louiseadennis

No problem. When I merged master into my development branch I bodged some of the conflict resolution and the resulting broken mess has been sitting on dream hack depressing me which means there's been no progress.

@afuna
Dreamwidth Studios member
afuna commented Feb 1, 2015

Sorry to hear that @annariel! It's almost there so I'll just finish this up :)

@afuna afuna self-assigned this Feb 1, 2015
@afuna afuna added a commit to afuna/dw-free that referenced this pull request Feb 3, 2015
@afuna afuna [#312] Allow multiple stickies
* Allows multiple stickies to be set both in the post forms via a sticky
  module, and in the Display tab under Manage Account

* Allows for different sticky allocations depending upon account type
  and attempts to handle switching between account types elegantly

Much thanks to @annariel for her initial work on this.
4abd8fa
@afuna afuna added a commit to afuna/dw-free that referenced this pull request Feb 3, 2015
@afuna afuna [#312] Allow multiple stickies
* Allows multiple stickies to be set both in the post forms via a sticky
  module, and in the Display tab under Manage Account

* Allows for different sticky allocations depending upon account type
  and attempts to handle switching between account types elegantly

Much thanks to @annariel for her initial work on this.
6ccf1f3
@afuna afuna added a commit to afuna/dw-free that referenced this pull request Feb 3, 2015
@afuna afuna [#312] Allow multiple stickies
* Allows multiple stickies to be set both in the post forms via a sticky
  module, and in the Display tab under Manage Account

* Allows for different sticky allocations depending upon account type
  and attempts to handle switching between account types elegantly

Much thanks to @annariel for her initial work on this.
799924d
@zorkian
Dreamwidth Studios member
zorkian commented Feb 3, 2015

Closing this, as it's been superseded by @afuna's last PR.

Thanks for writing on this @annariel! Yay! :D

@zorkian zorkian closed this Feb 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.