CDSK-583 - add every sourcestamps to link in MyBuilds. CDSK-812 - add revision column #335
Conversation
| day_count = 7 | ||
| example_data = [ | ||
| fakedb.SourceStamp(id=1, branch="HEAD", sourcestampsetid=2, codebase="fmod"), | ||
| fakedb.SourceStamp(id=2, branch="trunk", sourcestampsetid=3, codebase="bar"), |
There was a problem hiding this comment.
Make sure to add a test for when there are multiple sourcestamps attached to a single sourcestampsetid
|
One link with every sourcestamps
PS. Column with branch will be added in another PR. |
warcholprzemo
changed the title
warcholprzemo
force-pushed
the
1864-add-branch-into-MyBuilds
branch
from
975df99
to
a8e5e6a
Compare
warcholprzemo
force-pushed
the
1864-add-branch-into-MyBuilds
branch
from
a8e5e6a
to
3d5820a
Compare
| @@ -302,5 +302,6 @@ def _minimal_bdict(row, botmaster): | |||
| project=botmaster.getBuilderConfig(row.buildrequests_buildername).project, | |||
| slavename=row.builds_slavename, | |||
| submitted_at=mkdt(row.buildrequests_submitted_at), | |||
| complete_at=mkdt(row.buildrequests_complete_at) | |||
| complete_at=mkdt(row.buildrequests_complete_at), | |||
| sourcestampsetid=row.buildsets_sourcestampsetid, | |||
There was a problem hiding this comment.
Are you sure we need it as minimal information?
I think the sourcestampsetid is required for only one method, so maybe we should add this information only there.
There was a problem hiding this comment.
_minimal_bdict is used only by getLastBuildsOwnedBy and I just added the line into existing code. I agree, that method is ugly because:
- is staticmethod
- is unnecessary
| def thd(conn): | ||
| sourcestamps_tbl = self.db.model.sourcestamps | ||
|
|
||
| query = sa.select(columns=[sourcestamps_tbl.c.sourcestampsetid, |
| @@ -16,7 +17,21 @@ def content(self, req, cxt): | |||
| master.config.myBuildDaysCount, | |||
| ) | |||
|
|
|||
| cxt['builds'] = builds | |||
| builds_by_ssid = {} | |||
There was a problem hiding this comment.
I am really not sure, that it is a correct place for this operation. As we can read from djangobook (https://djangobook.com/model-view-controller-design-pattern/), the controller should decide who can have access to the model and defer it to a template. This code, in my personal opinion, should be put in models.
There was a problem hiding this comment.
In my opinion, the controller is a better place, because we can use DB functions again regardless final html code. In classical MVC (like Ruby on Rails) we can use more logic on view, but in Jinja this is ugly (we have to write dedicated tag / filter).
Maybe I should move this for-loop to another method and then I can test it.
@campos-ddc What do you think?
There was a problem hiding this comment.
I don't really care, perhaps try to follow the same pattern that the rest of katana code uses (if there is any)
| def thd(conn): | ||
| sourcestamps_tbl = self.db.model.sourcestamps | ||
|
|
||
| query = sa.select(columns=[sourcestamps_tbl.c.sourcestampsetid, |
| @@ -16,7 +17,21 @@ def content(self, req, cxt): | |||
| master.config.myBuildDaysCount, | |||
| ) | |||
|
|
|||
| cxt['builds'] = builds | |||
| builds_by_ssid = {} | |||
There was a problem hiding this comment.
In my opinion, the controller is a better place, because we can use DB functions again regardless final html code. In classical MVC (like Ruby on Rails) we can use more logic on view, but in Jinja this is ugly (we have to write dedicated tag / filter).
Maybe I should move this for-loop to another method and then I can test it.
@campos-ddc What do you think?
| 'branch': row.branch, | ||
| 'codebase': row.codebase, | ||
| 'revision': row.revision, | ||
| 'short_revision': row.revision[:12], |
There was a problem hiding this comment.
Why 12? Maybe we should move it to the configuration of the master? This twelve is a magic number.
There was a problem hiding this comment.
That is what Katana is using right now (13 characters for "short" revision).
I do agree on the magic thing, so it should either be a constant somewhere (I bet that this magic 12 appears in other places), or configurable as Kamil said.
I don't see any benefit in changing the number, so if we did move it to some config, it would still be the same 12. That being the case, I think that a constant is probably enough.
…umn-in-MyBuilds CDSK-812 - add revision column to MyBuilds
warcholprzemo
changed the title
|
Revision column from merged PR:
|
For now, I added next join into getLastBuildsOwnedBy.
I need to join to sourcestamps_tbl, because there are branch and codebase for a build.
Maybe we will change table to build_user and remove my join, but we need to add 2 extra columns there.
So this PR waits for #327 (new build_user table) and decision