TODO list of non-essential things

[Pa3u3u]

The list of things that should be done at some point in time but are not essential, just polishing:

• cooler /403 would be nice,
• also, link to home / back from /403 ?
• @FiXerCz , can you add similar note that is in Trips to Excursions that cannot be deleted?
  • note that it only shows up for STAFF and if there are excursions that cannot be deleted,
• under logout or generally somewhere on main page there should be links to delete and edit the account (currently there is no way for USER|STAFF to do that except directly typing /accounts/edit/<my_id>)
• favicon! :cake:

You are welcome to propose your own ideas.

[Pa3u3u]

what method?

[FiXerCz]

Sorry, I did not see your message. I had mapping for 403 error page in AuthController. You have added it for a bit different mapping to ErrorController. So i removed it from AuthController, as now it makes more sense in ErrorController.

I have also noticed that Chrome + IE have different value for locale than Firefox, so we need to change comparisons for deciding what template to include to this:

${pageContext.request.locale == 'cs' || pageContext.request.locale == 'cs_CZ'}

That works in all 3 browsers.

[Pa3u3u]

Dammit ... ok, I will change it when I finish examples (20 Excursions, 7 Trips, so far 7 Customers) ... enough? :D

[FiXerCz]

I think it is enough, but if you are enjoying yourself, then by all means, keep it on coming :D:D

[Pa3u3u]

Did you change some of *.jsp files?

Also, can you czech the check version of those error pages? ( :wink: )

[FiXerCz]

In my last commit I have changed index.jsp and account/form.jsp.

Sure, I can look at the translations.

:D:D You have added Wherever the wind takes me. I forgot to ask you to that, but obviously I did not have to.

I have also just tested authentication for REST. Works great, nice job! I wonder how many other teams will have authentication there ;)

[Pa3u3u]

But I have forgot to add the discount tours to Alpha Centauri Bb :smiley: And I have found another error (the condition when to disable Trips' delete button is inversed), so I will fix both

[FiXerCz]

I found only minor mistakes. Can I correct them directly? It would be easier.

Should I also add the conditions to error pages for checking for "cs_CZ" locale?

EDIT: "iPad owners only" :D:D That makes me proud to own an Android tablet.

[Pa3u3u]

Well, I found more compact condition:

<%@ taglib prefix="fn" uri="http://java.sun.com/jsp/jstl/functions" %>

<c:when test="${fn:startsWith(pageContext.response.locale, 'cs')}"> ... </c:when>

which works for any cs_CZ or cs_CZ.utf8 etc., but it does not matter, if you fixed it already

[FiXerCz]

Alright, I can use that. I knew there will be something to check the prefix but i was too lazy to look for that :D

I will push it along with grammar corrections.

[FiXerCz]

Ok, it is done.

[Pa3u3u]

Ad "iPad owners only": Did you notice the price? :smiley:

Also, it is quite strange that you can login into REST API as an USER and delete trips and excursions... :smiley:

[Pa3u3u]

... and the reason is that certain team leader is a moron and forgot to add @Secured annotations on Service layer.

[FiXerCz]

I did notice price. And the travel guide on that excursion should be called Bender. It relates to iPhones, but what the hell.. :smiley:

[Pa3u3u]

:smiley: Ok, so, what's left?

[FiXerCz]

I think we are done. All requirements are met. Maybe we can do final check, if everything works (click it through) and try clean build (repo checkout and build?). I have also seen some commented out code in JPAAccountDAOTest. Does anything else come in mind?

I will add my slides with my assesment of this subject tomorow, as tomorow I will be studying for java exam so I will do it both.

Btw I have just read requirements again and I think that deadline was an hour ago :D But hopefully it won't be as strict as previous checkpoints that had explicit date and time.

[FiXerCz]

Oh and readme should be updated about CLI auth. I think that just copying your entry from auth issue shoud do.

[Pa3u3u]

Yes, I noticed it too... well, we can argue that we didn't know the precise time of the deadline, since we got the exact time only on friday... but meeh. We have 40 points, what can go wrong? :smiley:

All right, let's make it faster, you check the Dao, Service and API (also documentation!), I will check the rest (Web, CLI). And I will update the Readme as well.

[FiXerCz]

Okay.

[Pa3u3u]

I have noticed a very odd thing:

• some files use CRLF line endigs, some only LF,
• some files do not contain trailing EOL sequence,
• there are many files with spaces after the last printable character

While these are not problems (well, the second one actually is, because a text file must end with EOL by definition), I have a script at work that can fix this all at once ... I think I may run it after we check the code :smiley:

[Pa3u3u]

One more thing: when you are done, you can recheck warnings automatically in NetBeans:

1. select the root module (SurrealTravel),
2. Navigate > Inspect
3. Select "All analyzers" and run.

You can ignore all "Missing Javadoc", but for instance, in AccountDTO there is a warning about redundant if statement.

[FiXerCz]

Will do. I will commit per module to avoid one giant commit blob.

[Pa3u3u]

:scream: how many changes did you do?

[FiXerCz]

Well it is mostly code formatting and comment typos changes, but in some tests I had to remove quite a lot of unused variables :)

And I have that checking under Source > Inspect :)

[Pa3u3u]

Oooo ... yes, sorry, it's under Source here in LinuxLand as well, I'm just too tired to think properly right now :smiley:

[Pa3u3u]

I've finished CLI and Web (there were not too many problems), so I will fix that warning and check API as well.

[FiXerCz]

I have just pushed DAO. Ok, I will do Service and leave API to you.

[FiXerCz]

Overriden methods for Default_Service have documentation, although they are documented in their interfaces in API. Shoud I remove docs from Service?

[Pa3u3u]

Mmm ... we created them copy-paste I remember :smiley: well, it's not a mistake to have documentation on @Override methods, so I will leave it up to you.

[FiXerCz]

That is odd, I have commited DefaultReservationService and AccountServiceTest and whenever I try to push I get notified there is conflict. Odd..

[Pa3u3u]

which file is conflicting? O_o try pull first (Git will not allow you to push if your have a staged commit, but master is ahead).

[FiXerCz]

When I try pull it also says that conflict would arise.

How can I revert my local commit (not pushed)? Git > Remote > Show Outgoing > Revert Commit?

I dont want to fuck anything up.

[FiXerCz]

Yeah, incomming show your changes in API so it is just that master is one commit ahead. If I push it, it should merge correctly, no? It is in separate modules.

[Pa3u3u]

but which file would be in conflict? O_o doesn't it say? Reverting a commit WILL be a problem, mmm, make backup of the Service layer somewhere, then revert it, pull, and copy your changed files back

[Pa3u3u]

(If you were using command-line GIT, I think it would be easier, as it usually precisely shows you where's the problem and how to fix it :smiley:

[FiXerCz]

It does not say which file. And there should not be any, you have changed API and I have changes in Service. Funny bussiness. I will try to revert my local commit, hopefully it will not revert stuff you have pushed to API.

[Pa3u3u]

We can still revert the revert :smiley:

[FiXerCz]

Well that commit chain is weird like a motherfucker, but I somehow got it there. Almost not worth the effort, since I did not change almost anything.

[Pa3u3u]

There's no conflict :smiley: I think it was the NetBeans' Git client wrongly interpreting what I said (the underlying Git will not allow you to push if there's a new commit in the master).

[FiXerCz]

Ok, so now you will run that magic script to fix wrong EOL etc and change readme and we are done, I suppose?

[Pa3u3u]

Almost. I randomly tried something and realized that when you remove your account, the customer remains for some reason :smiley:

[FiXerCz]

Whatever, lets keep it for "accounting" reasons :D Nice little economy feature.

[Pa3u3u]

I see why. The "CustomerService.deleteById" is secured for admins only

[Pa3u3u]

I will make it "for users only when ID's match or admins" :smiley:

[FiXerCz]

Oh, I have automatically set everything for Customers for admins only except for creating (cause of reservations). At that time I did not know we will be removing accounts and after it was decided, I must have forgotten to change it. Sry.

[FiXerCz]

But removing customer during removal of account might scream, if there are reservations for that customer. You would need to remove probably those first.

[Pa3u3u]

I know, the original code even know about it :smiley: it failed in the last step, when removing the customer

[Pa3u3u]

Ok, fuck it, I can't delete customer that is referenced by account and I can't delete account that is referenced by customer. I would have to override some service methods, that I don't have time for. So, the customer will remain :smiley:

[FiXerCz]

I said it before, lets label it economy and accounting feature :-) Removing account keeps customers for checking of... whatever needs to be checked. History of reservations or sth :smiley:

[Pa3u3u]

1. this is 100th comment! :balloon: :tada: :balloon: :tada: :balloon: :tada:
2. out of 181 files, 167 were somehow erroneous :smiley:

You can see the list here After [FIXD], the list of 3 chars contains what was fixed:

• e - missing EOL at the end of file
• c - CRLF instead of LF
• w - whitespaces at the end of line

Btw, the command was

./fixsrc.pl --all --ignore-path "./.git" --ignore-path "lib" --ignore-file "*.png" --ignore-file "*.gif" -v -v --level 3 --color=always > ~/fixsrc.log

I never though I'd actually use this script outside of work :smiley: I'm just going to test it didn't do anything wrong