Opened 10 years ago

Closed 10 years ago

#11470 closed enhancement (duplicate)

Re-enable at symbol in notebook username

Reported by: kcrisman Owned by: jason, mpatel, was
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: notebook Keywords:
Cc: jason Merged in:
Authors: Reviewers: Karl-Dieter Crisman
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

We had to disable the '@' symbol in the notebook because it breaks TinyMCE.

See #11343 and the links in this thread on sage-notebook for further work.

Fixed in flask-sagenb.

Attachments (5)

Screen shot 2011-06-22 at 9.16.42 PM.png (131.8 KB) - added by rkirov 10 years ago.
mac os - chrome 12
Screen shot 2011-06-22 at 9.20.32 PM.png (150.7 KB) - added by rkirov 10 years ago.
mac os - firefox 3.6
Screen shot 2011-06-23 at 8.57.43 AM.png (58.4 KB) - added by kcrisman 10 years ago.
Mac OS 10.6 with Safari 5, reopening existing text cell
Screen shot 2011-06-23 at 8.58.09 AM.png (64.2 KB) - added by kcrisman 10 years ago.
Mac OS 10.6 with Safari 5, opening new text cell
Screen shot 2011-06-23 at 6.17.33 PM.png (251.0 KB) - added by rkirov 10 years ago.
mac os - safari - problem with error stack

Download all attachments as: .zip

Change History (49)

comment:1 Changed 10 years ago by rkirov

no such problem in flask. see http://code.google.com/p/sagenb/issues/detail?id=26&can=1 you can test on sagenb.org

comment:2 Changed 10 years ago by kcrisman

Not true, at least not at demo2.sagenb.org or sagenb.org (I tried both)

http://demo2.sagenb.org/home/b%40b/0/

is the name but it still causes problems. You can try this with password 'abc123'. Note that the user name at the top (next to Toggle) is still b@b, this could be relevant?

So *please* either fix it or merge #11343 into the flask "main" body. Many, many beginning users like to use their email addresses for usernames.

Changed 10 years ago by rkirov

mac os - chrome 12

Changed 10 years ago by rkirov

mac os - firefox 3.6

comment:3 Changed 10 years ago by rkirov

hmmm, can't reproduce the bug in either chrome or firefox on mac os *screenshots attached*. Can we get more people to test it?

comment:4 Changed 10 years ago by kini

works for me (see bottom of the screenshot for system info)

comment:6 Changed 10 years ago by kini

Works in Chromium too, btw.

comment:7 Changed 10 years ago by kcrisman

Very interesting. I use Safari. Hang on a second...

Changed 10 years ago by kcrisman

Mac OS 10.6 with Safari 5, reopening existing text cell

Changed 10 years ago by kcrisman

Mac OS 10.6 with Safari 5, opening new text cell

comment:8 Changed 10 years ago by kcrisman

I can confirm that FF works fine. Safari is in bad shape.

So what should we do regarding #11343? I'm a little torn, as this is browser-specific, but a lot of current Sage folks use Safari since they are on Mac OS X.

Changed 10 years ago by rkirov

mac os - safari - problem with error stack

comment:9 Changed 10 years ago by rkirov

aha, I can reproduce this on Safari 5. You can see in the error trace below that TinyMCE is making the wrong url requests with break everything /home/b@/... . I will try to fix it, if not will incorporate kcrisman's patch before the spkg, gets made.

Of course, help is welcome.

comment:10 Changed 10 years ago by kini

It's actually /home/b@demo2.sagenb.org/... (where it should be /home/b%40b/...), which seems to tie this to this (which I think kcrisman found earlier)...

comment:11 Changed 10 years ago by kini

Oops, link in the OP of that is broken - the original bug report is here.

comment:12 follow-up: Changed 10 years ago by kini

So is this bug invalid, or what? #11343 has positive review. Should I merge it into the flask notebook?

comment:13 in reply to: ↑ 12 Changed 10 years ago by kcrisman

Replying to kini:

So is this bug invalid, or what? #11343 has positive review. Should I merge it into the flask notebook?

See the Google code bug I opened recently. It is not in the flask notebook yet. My feeling is that something like it should be, though.

comment:14 follow-ups: Changed 10 years ago by jason

As far as I remember, I never merged anything into the flask notebook that disabled the @ sign in usernames because it wasn't a problem there. However, I just checked with Safari, and indeed, it appears that the URLs still contain an @ sign and on Safari, with the current sagenb, the tinymce doesn't work.

I'd rather just fix the URL encoding problem with the @ sign in the flask notebook, rather than merging this.

comment:15 in reply to: ↑ 14 Changed 10 years ago by kcrisman

Replying to jason:

As far as I remember, I never merged anything into the flask notebook that disabled the @ sign in usernames because it wasn't a problem there. However, I just checked with Safari, and indeed, it appears that the URLs still contain an @ sign and on Safari, with the current sagenb, the tinymce doesn't work.

I'd rather just fix the URL encoding problem with the @ sign in the flask notebook, rather than merging this.

Well, this ticket is for re-enabling :) But you mean merging #11343.

Naturally, if you can just fix it, that would be even better. Is that possible? (I mean without fixing Safari or TinyMCE? I assume that sagenb itself is not the problem.)

comment:16 in reply to: ↑ 14 Changed 10 years ago by dimpase

Replying to jason:

As far as I remember, I never merged anything into the flask notebook that disabled the @ sign in usernames because it wasn't a problem there. However, I just checked with Safari, and indeed, it appears that the URLs still contain an @ sign and on Safari, with the current sagenb, the tinymce doesn't work.

I'd rather just fix the URL encoding problem with the @ sign in the flask notebook, rather than merging this.

I would rather ignore these little browser specific quirks for the time being, as they come and go as browsers get updated.

comment:17 Changed 10 years ago by jason

Is it browser-specific? I was under the impression it wasn't.

comment:18 follow-up: Changed 10 years ago by jason

Ah, I see above that FF works.

Well, I don't know if this is a browser-specific bug, or if there is a real bug that FF is being lenient on. If the latter, we should still fix our codebase, for sure.

comment:19 in reply to: ↑ 18 Changed 10 years ago by dimpase

  • Milestone set to sage-wishlist
  • Priority changed from major to minor

Replying to jason:

Ah, I see above that FF works.

Well, I don't know if this is a browser-specific bug, or if there is a real bug that FF is being lenient on. If the latter, we should still fix our codebase, for sure.

#11343 (the 3-months old part of the discussion there) is quite informative about the browser-specific reasons for this. As you see, #11343 disables new registrations using @, while still allowing the existing ones to work. So you merged #11343 into the flask version, right? Then I would like to put this ticket on the wish-list, and be done with it for the time being.

comment:20 Changed 10 years ago by kini

Well, Rado reverted #11343 in the flask notebook, thinking it only applied to the old notebook (due to him testing on firefox), thus the creation of this ticket to track that fact. After we determined it is broken on Safari, nobody un-reverted #11343 in the flask notebook. Hence my bump.

comment:21 Changed 10 years ago by jason

Isn't the solution just a matter of URL-encoding the username when making worksheet URLs?

comment:22 Changed 10 years ago by jason

  • Status changed from new to needs_review

I think I fixed the underlying issue in this pull request: https://github.com/sagemath/sagenb/pull/11

comment:23 Changed 10 years ago by jason

By the way, I verified that this problem happens in Chrome and Firefox on OSX.

comment:24 follow-up: Changed 10 years ago by kini

Awesome! Strange that it turned out to be OS X and not Safari specifically. I wonder what could be happening at the OS level to cause it to work fine on Linux. Just to be sure, maybe we should wait for kcrisman to verify that this fixes the problem, since before jason's confirmation he was the only person to see this bug (afaik).

comment:25 in reply to: ↑ 24 ; follow-up: Changed 10 years ago by dimpase

Replying to kini:

Awesome! Strange that it turned out to be OS X and not Safari specifically. I wonder what could be happening at the OS level to cause it to work fine on Linux. Just to be sure, maybe we should wait for kcrisman to verify that this fixes the problem, since before jason's confirmation he was the only person to see this bug (afaik).

well, I can try to see whether I can see the bug on 4.8. I didn't manage to reproduce it on when working on #11343.

comment:26 Changed 10 years ago by jason

I wonder if the unpredictability has to do with caching, rather than the OS or browser. The bit of code is run when the tinymce file is initialized in order to determine where to fetch the tinymce files from. If that initialization is already set somewhere, it may not cause a problem. It may also be a difference between browsers/OS, as it asks the window for the current URL, and maybe there are differences there.

comment:27 Changed 10 years ago by jason

In fact, I remember not seeing the problem once on Chrome, but then later seeing it. That points to caching.

comment:28 in reply to: ↑ 25 Changed 10 years ago by kcrisman

Replying to dimpase:

Replying to kini:

Awesome! Strange that it turned out to be OS X and not Safari specifically. I wonder what could be happening at the OS level to cause it to work fine on Linux. Just to be sure, maybe we should wait for kcrisman to verify that this fixes the problem, since before jason's confirmation he was the only person to see this bug (afaik).

well, I can try to see whether I can see the bug on 4.8. I didn't manage to reproduce it on when working on #11343.

It looks like there isn't really a "patch" to apply there, and in any case I can't test it locally because my Sage installs are all ones which already disallow '@' from #11343! And I'm definitely not going to try to sort out installing the flask sagenb at this point. Sorry.

But if this is updated on sagenb or someplace like that I'd be happy to test a before and after for you.

Also, I can definitely confirm that it wasn't just me that saw it. A number of students this semester saw it, because they had created usernames on our server before we upgraded, and tried to use them when I was teaching them how to use LaTeX in various places, including Sage. I feel like there was at least one sage-support request, but I know I wouldn't find it right now.

Thanks very much for tracking this down, Jason!

comment:30 follow-up: Changed 10 years ago by kcrisman

Yes, I was able to view the second part of the diff before, though the first part won't view on github and seems far too long on your link as well. My point is that it's not a patch I can import, particularly not to the old sagenb.

Anyway, the bigger point is that I have no way of testing it locally.

comment:31 in reply to: ↑ 30 Changed 10 years ago by dimpase

Replying to kcrisman:

Yes, I was able to view the second part of the diff before, though the first part won't view on github and seems far too long on your link as well. My point is that it's not a patch I can import, particularly not to the old sagenb.

Anyway, the bigger point is that I have no way of testing it locally.

Hi Karl-Dieter,

The 1st part of the diff is irrelevant, because it is the diff of the file obtained from the 2nd file, by some kind of preprocessing, called minification, improving the performance (something akin to .pyc vs .py files in Python).

Jason, by the way, what is the reason to distribute the 1st file, shouldn't it be created at installation time?

For the purposes of testing, the 1st file can be replaced with the 2nd at no loss.

You can take the diff, and apply it using good old patch tool, too.

You can test with the old sagenb, by installing the new sagenb and applying the patch via git, then replacing the tiny_mce you have in the old sagenb with this new one, that is, the whole devel/sagenb/sagenb/data/tiny_mce/

Or, even easier, unpack the new sagenb spkg, and apply the patch manually, then replace the tiny_mce you have in the old sagenb with this new one, that is, the whole devel/sagenb/sagenb/data/tiny_mce/

comment:32 follow-up: Changed 10 years ago by jason

To review it, it's better just to download the two files and replace your versions with them. To do that, go to the pull request, click on "Diff", and then for each file, click on "view file". Then click the "raw" versions of the files at that commit. Here are links to the files:

https://raw.github.com/jasongrout/sagenb/5b1b4c50437db1aa3d912040d29dd33c5e24d80d/sagenb/data/tiny_mce/tiny_mce.js

https://raw.github.com/jasongrout/sagenb/5b1b4c50437db1aa3d912040d29dd33c5e24d80d/sagenb/data/tiny_mce/tiny_mce_src.js

To answer dimpase: The entire tinymce directory is just an uncompressed tinymce release tarball. I'd rather not duplicate their release process and minifying, but rather would like to just use their straight release.

comment:33 in reply to: ↑ 32 Changed 10 years ago by dimpase

Replying to jason:

To answer dimpase: The entire tinymce directory is just an uncompressed tinymce release tarball. I'd rather not duplicate their release process and minifying, but rather would like to just use their straight release.

Hi Jason, I wonder how do you do the minifying. Thanks, Dima

comment:34 follow-up: Changed 10 years ago by jason

I don't do any minifying. I just extract the tinymce tarball I downloaded from the website. The minified version is already included in the standard tinymce release tarball.

comment:35 in reply to: ↑ 34 Changed 10 years ago by dimpase

Replying to jason:

I don't do any minifying. I just extract the tinymce tarball I downloaded from the website. The minified version is already included in the standard tinymce release tarball.

Well, but where does the diff for the minified file come from?

comment:36 follow-up: Changed 10 years ago by jason

I hand-modified both files, the minified version since it's the version we actually send to the browser, and the _src file so that it would be easier to see what the changes were.

comment:37 in reply to: ↑ 36 Changed 10 years ago by dimpase

Replying to jason:

I hand-modified both files, the minified version since it's the version we actually send to the browser, and the _src file so that it would be easier to see what the changes were.

Wow, it comes close to fixing a bug in a, say, C program by editing the .o file. :–) I certainly would have looked for a tool to do it...

Shouldn't we minify the rest of the js code in sagenb, for the same reason, minimizing load times?

comment:38 Changed 10 years ago by kini

kcrisman, jason has applied the fix to test.sagenb.org. Can you give it a try?

comment:39 Changed 10 years ago by jason

I also had to restart sagenb.org, so the fix should be there too. Can you try it either on test.sagenb.org or just plain sagenb.org?

comment:40 Changed 10 years ago by jason

(remember to control-refresh or option-refresh or whatever so that your javascript cache gets overwritten)

comment:41 Changed 10 years ago by kcrisman

  • Status changed from needs_review to positive_review

Okay, I didn't even have to do anything with the cache - I had never seen that option before.

Anyway, works on both the test accounts here and at #11343. It does behave slightly different in terms of the default fonts and sizes, I think, but that is not very important. At least it works now! Thanks much.

comment:42 Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_info

What should I (as release manager) do with this? Is there anything to be merged or is it all in the flask-sagenb?

comment:43 Changed 10 years ago by jason

  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.

There is nothing to merge. We've already merged the fix to the underlying problem in flask-sagenb. I guess this isn't technically in Sage yet, so maybe this ticket should be closed when the flask notebook is merged. That is similar to how reported bugs in other upstream projects are handled, I think. When the new version of the upstream version is merged, the appropriate bugs are closed.

comment:44 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-wishlist to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_info to closed
Note: See TracTickets for help on using tickets.